Re: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
Rafael, As you pointed out, ata_tport_delete() should be after scsi_remove_host(), consistent with ata_tport_add() currently before ata_scsi_add_host(). Thanks for fixing it, Gwendal. On Mon, Nov 25, 2013 at 2:41 AM, Rafael J. Wysocki wrote: > > On Monday, November 25, 2013 12:11:54 PM Mika Westerberg wrote: > > On Sun, Nov 24, 2013 at 02:09:09AM +0100, Rafael J. Wysocki wrote: > > > On Sunday, November 24, 2013 12:36:03 AM Rafael J. Wysocki wrote: > > > > On Saturday, November 23, 2013 03:07:01 PM Greg Kroah-Hartman wrote: > > > > > On Sun, Nov 24, 2013 at 12:12:59AM +0100, Rafael J. Wysocki wrote: > > > > > > On Saturday, November 23, 2013 02:53:58 PM Greg Kroah-Hartman wrote: > > > > > > [...] > > > > > > > > > > > > > > > > > I can revert Mika's patch, as it would be good to catch these > > > > > > > kinds of > > > > > > > errors. > > > > > > > > > > > > Then we'll need to untangle the SATA/SCSI mess triggered by Bjorn in > > > > > > https://bugzilla.kernel.org/show_bug.cgi?id=65281. ;-) > > > > > > > > > > I have no objection to fixing that, the scsi sysfs handling is "odd" > > > > > to > > > > > say the least... > > > > > > > > > > If someone can unwind it, that would be great to see happen... > > > > > > > > Well, if I'm bored to death during the xmas holidays, I may look into > > > > that. > > > > > > In fact, I'm not exactly sure why ata_port_detach() calls > > > ata_tport_delete() > > > before scsi_remove_host()? Is there any particular reason? Because that > > > doesn't seem to be exactly right ... > > > > I tried so that I have your 'PCI: Move device_del() from pci_stop_dev() to > > pci_destroy_dev()' applied and then I did following change as you > > suggested. > > > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > > index 81a94a3919db..07a03f93d640 100644 > > --- a/drivers/ata/libata-core.c > > +++ b/drivers/ata/libata-core.c > > @@ -6304,10 +6304,10 @@ static void ata_port_detach(struct ata_port *ap) > > for (i = 0; i < SATA_PMP_MAX_PORTS; i++) > > ata_tlink_delete(>pmp_link[i]); > > } > > - ata_tport_delete(ap); > > - > > /* remove the associated SCSI host */ > > scsi_remove_host(ap->scsi_host); > > + > > + ata_tport_delete(ap); > > } > > > > /** > > > > After both patches are applied the warnings are gone :) However, looks like > > both are needed since if I only apply one or another, I still get warnings. > > Yes, they are both needed. :-) > > OK, I'll submit the above upstream if you don't mind. > > Cheers, R. > > -- > To unsubscribe from this list: send the line "unsubscribe linux-ide" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Monday, November 25, 2013 10:29:00 AM James Bottomley wrote: > On Fri, 2013-11-22 at 11:02 -0500, Tejun Heo wrote: > > Hello, > > > > On Fri, Nov 22, 2013 at 08:43:55AM -0700, Bjorn Helgaas wrote: > > > > So, we do have cases where the parent is removed before the child. I > > > > suppose the parent pci bridge is removed already? AFAICS this > > > > shouldn't break anything but people did seem to expect the removals to > > > > be ordered from child to parent. Bjorn, is this something you expect > > > > to happened? > > > > > > I do not expect a PCI bridge to be removed before the devices below > > > it. We should be removing all the children before removing the parent > > > bridge. > > > > > > But is this related to PCI? I don't see the connection yet. I tried > > > > I'm not sure. It was from thunderbolt and nobody is reporting it on > > other interconnects, so it could be. > > > > > to look into this a bit (my notes are at > > > https://bugzilla.kernel.org/show_bug.cgi?id=65281), but I haven't > > > figured out the big-picture problem yet. > > > > > > I don't have warm fuzzies that adding a "have we already removed this" > > > check is the best resolution, but maybe that's just because I don't > > > understand the problem. > > > > Yeah, the whole thing is sorta pointless. Just issuing removal and > > continuing on should do, IMHO. > > I'd go for that as well. We have huge problems with the _del calls > because visibility is strict hierarchy and it's not always easy to work > out who's underneath us. > > It's going to be really annoying when refcounting works perfectly for > objects, so you can just do puts in any order, but you have to have > _del() called to remove subordinate objects before their parent. Well, that would be fine and dandy, but device_del() calls bus_remove_device() which in turn runs device_release_driver() and the order in which *these* things are done actually matters in general. So yes, after we have released all of the drivers in question, we can do _del() right before the final _put() in any order just fine. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Fri, 2013-11-22 at 11:02 -0500, Tejun Heo wrote: > Hello, > > On Fri, Nov 22, 2013 at 08:43:55AM -0700, Bjorn Helgaas wrote: > > > So, we do have cases where the parent is removed before the child. I > > > suppose the parent pci bridge is removed already? AFAICS this > > > shouldn't break anything but people did seem to expect the removals to > > > be ordered from child to parent. Bjorn, is this something you expect > > > to happened? > > > > I do not expect a PCI bridge to be removed before the devices below > > it. We should be removing all the children before removing the parent > > bridge. > > > > But is this related to PCI? I don't see the connection yet. I tried > > I'm not sure. It was from thunderbolt and nobody is reporting it on > other interconnects, so it could be. > > > to look into this a bit (my notes are at > > https://bugzilla.kernel.org/show_bug.cgi?id=65281), but I haven't > > figured out the big-picture problem yet. > > > > I don't have warm fuzzies that adding a "have we already removed this" > > check is the best resolution, but maybe that's just because I don't > > understand the problem. > > Yeah, the whole thing is sorta pointless. Just issuing removal and > continuing on should do, IMHO. I'd go for that as well. We have huge problems with the _del calls because visibility is strict hierarchy and it's not always easy to work out who's underneath us. It's going to be really annoying when refcounting works perfectly for objects, so you can just do puts in any order, but you have to have _del() called to remove subordinate objects before their parent. James -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Monday, November 25, 2013 12:11:54 PM Mika Westerberg wrote: > On Sun, Nov 24, 2013 at 02:09:09AM +0100, Rafael J. Wysocki wrote: > > On Sunday, November 24, 2013 12:36:03 AM Rafael J. Wysocki wrote: > > > On Saturday, November 23, 2013 03:07:01 PM Greg Kroah-Hartman wrote: > > > > On Sun, Nov 24, 2013 at 12:12:59AM +0100, Rafael J. Wysocki wrote: > > > > > On Saturday, November 23, 2013 02:53:58 PM Greg Kroah-Hartman wrote: > > > > [...] > > > > > > > > > > > > > > I can revert Mika's patch, as it would be good to catch these kinds > > > > > > of > > > > > > errors. > > > > > > > > > > Then we'll need to untangle the SATA/SCSI mess triggered by Bjorn in > > > > > https://bugzilla.kernel.org/show_bug.cgi?id=65281. ;-) > > > > > > > > I have no objection to fixing that, the scsi sysfs handling is "odd" to > > > > say the least... > > > > > > > > If someone can unwind it, that would be great to see happen... > > > > > > Well, if I'm bored to death during the xmas holidays, I may look into > > > that. > > > > In fact, I'm not exactly sure why ata_port_detach() calls ata_tport_delete() > > before scsi_remove_host()? Is there any particular reason? Because that > > doesn't seem to be exactly right ... > > I tried so that I have your 'PCI: Move device_del() from pci_stop_dev() to > pci_destroy_dev()' applied and then I did following change as you > suggested. > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index 81a94a3919db..07a03f93d640 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -6304,10 +6304,10 @@ static void ata_port_detach(struct ata_port *ap) > for (i = 0; i < SATA_PMP_MAX_PORTS; i++) > ata_tlink_delete(>pmp_link[i]); > } > - ata_tport_delete(ap); > - > /* remove the associated SCSI host */ > scsi_remove_host(ap->scsi_host); > + > + ata_tport_delete(ap); > } > > /** > > After both patches are applied the warnings are gone :) However, looks like > both are needed since if I only apply one or another, I still get warnings. Yes, they are both needed. :-) OK, I'll submit the above upstream if you don't mind. Cheers, R. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Sun, Nov 24, 2013 at 02:09:09AM +0100, Rafael J. Wysocki wrote: > On Sunday, November 24, 2013 12:36:03 AM Rafael J. Wysocki wrote: > > On Saturday, November 23, 2013 03:07:01 PM Greg Kroah-Hartman wrote: > > > On Sun, Nov 24, 2013 at 12:12:59AM +0100, Rafael J. Wysocki wrote: > > > > On Saturday, November 23, 2013 02:53:58 PM Greg Kroah-Hartman wrote: > > [...] > > > > > > > > > > > I can revert Mika's patch, as it would be good to catch these kinds of > > > > > errors. > > > > > > > > Then we'll need to untangle the SATA/SCSI mess triggered by Bjorn in > > > > https://bugzilla.kernel.org/show_bug.cgi?id=65281. ;-) > > > > > > I have no objection to fixing that, the scsi sysfs handling is "odd" to > > > say the least... > > > > > > If someone can unwind it, that would be great to see happen... > > > > Well, if I'm bored to death during the xmas holidays, I may look into that. > > In fact, I'm not exactly sure why ata_port_detach() calls ata_tport_delete() > before scsi_remove_host()? Is there any particular reason? Because that > doesn't seem to be exactly right ... I tried so that I have your 'PCI: Move device_del() from pci_stop_dev() to pci_destroy_dev()' applied and then I did following change as you suggested. diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 81a94a3919db..07a03f93d640 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -6304,10 +6304,10 @@ static void ata_port_detach(struct ata_port *ap) for (i = 0; i < SATA_PMP_MAX_PORTS; i++) ata_tlink_delete(>pmp_link[i]); } - ata_tport_delete(ap); - /* remove the associated SCSI host */ scsi_remove_host(ap->scsi_host); + + ata_tport_delete(ap); } /** After both patches are applied the warnings are gone :) However, looks like both are needed since if I only apply one or another, I still get warnings. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Sun, Nov 24, 2013 at 02:09:09AM +0100, Rafael J. Wysocki wrote: On Sunday, November 24, 2013 12:36:03 AM Rafael J. Wysocki wrote: On Saturday, November 23, 2013 03:07:01 PM Greg Kroah-Hartman wrote: On Sun, Nov 24, 2013 at 12:12:59AM +0100, Rafael J. Wysocki wrote: On Saturday, November 23, 2013 02:53:58 PM Greg Kroah-Hartman wrote: [...] I can revert Mika's patch, as it would be good to catch these kinds of errors. Then we'll need to untangle the SATA/SCSI mess triggered by Bjorn in https://bugzilla.kernel.org/show_bug.cgi?id=65281. ;-) I have no objection to fixing that, the scsi sysfs handling is odd to say the least... If someone can unwind it, that would be great to see happen... Well, if I'm bored to death during the xmas holidays, I may look into that. In fact, I'm not exactly sure why ata_port_detach() calls ata_tport_delete() before scsi_remove_host()? Is there any particular reason? Because that doesn't seem to be exactly right ... I tried so that I have your 'PCI: Move device_del() from pci_stop_dev() to pci_destroy_dev()' applied and then I did following change as you suggested. diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 81a94a3919db..07a03f93d640 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -6304,10 +6304,10 @@ static void ata_port_detach(struct ata_port *ap) for (i = 0; i SATA_PMP_MAX_PORTS; i++) ata_tlink_delete(ap-pmp_link[i]); } - ata_tport_delete(ap); - /* remove the associated SCSI host */ scsi_remove_host(ap-scsi_host); + + ata_tport_delete(ap); } /** After both patches are applied the warnings are gone :) However, looks like both are needed since if I only apply one or another, I still get warnings. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Fri, 2013-11-22 at 11:02 -0500, Tejun Heo wrote: Hello, On Fri, Nov 22, 2013 at 08:43:55AM -0700, Bjorn Helgaas wrote: So, we do have cases where the parent is removed before the child. I suppose the parent pci bridge is removed already? AFAICS this shouldn't break anything but people did seem to expect the removals to be ordered from child to parent. Bjorn, is this something you expect to happened? I do not expect a PCI bridge to be removed before the devices below it. We should be removing all the children before removing the parent bridge. But is this related to PCI? I don't see the connection yet. I tried I'm not sure. It was from thunderbolt and nobody is reporting it on other interconnects, so it could be. to look into this a bit (my notes are at https://bugzilla.kernel.org/show_bug.cgi?id=65281), but I haven't figured out the big-picture problem yet. I don't have warm fuzzies that adding a have we already removed this check is the best resolution, but maybe that's just because I don't understand the problem. Yeah, the whole thing is sorta pointless. Just issuing removal and continuing on should do, IMHO. I'd go for that as well. We have huge problems with the _del calls because visibility is strict hierarchy and it's not always easy to work out who's underneath us. It's going to be really annoying when refcounting works perfectly for objects, so you can just do puts in any order, but you have to have _del() called to remove subordinate objects before their parent. James -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Monday, November 25, 2013 12:11:54 PM Mika Westerberg wrote: On Sun, Nov 24, 2013 at 02:09:09AM +0100, Rafael J. Wysocki wrote: On Sunday, November 24, 2013 12:36:03 AM Rafael J. Wysocki wrote: On Saturday, November 23, 2013 03:07:01 PM Greg Kroah-Hartman wrote: On Sun, Nov 24, 2013 at 12:12:59AM +0100, Rafael J. Wysocki wrote: On Saturday, November 23, 2013 02:53:58 PM Greg Kroah-Hartman wrote: [...] I can revert Mika's patch, as it would be good to catch these kinds of errors. Then we'll need to untangle the SATA/SCSI mess triggered by Bjorn in https://bugzilla.kernel.org/show_bug.cgi?id=65281. ;-) I have no objection to fixing that, the scsi sysfs handling is odd to say the least... If someone can unwind it, that would be great to see happen... Well, if I'm bored to death during the xmas holidays, I may look into that. In fact, I'm not exactly sure why ata_port_detach() calls ata_tport_delete() before scsi_remove_host()? Is there any particular reason? Because that doesn't seem to be exactly right ... I tried so that I have your 'PCI: Move device_del() from pci_stop_dev() to pci_destroy_dev()' applied and then I did following change as you suggested. diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 81a94a3919db..07a03f93d640 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -6304,10 +6304,10 @@ static void ata_port_detach(struct ata_port *ap) for (i = 0; i SATA_PMP_MAX_PORTS; i++) ata_tlink_delete(ap-pmp_link[i]); } - ata_tport_delete(ap); - /* remove the associated SCSI host */ scsi_remove_host(ap-scsi_host); + + ata_tport_delete(ap); } /** After both patches are applied the warnings are gone :) However, looks like both are needed since if I only apply one or another, I still get warnings. Yes, they are both needed. :-) OK, I'll submit the above upstream if you don't mind. Cheers, R. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Monday, November 25, 2013 10:29:00 AM James Bottomley wrote: On Fri, 2013-11-22 at 11:02 -0500, Tejun Heo wrote: Hello, On Fri, Nov 22, 2013 at 08:43:55AM -0700, Bjorn Helgaas wrote: So, we do have cases where the parent is removed before the child. I suppose the parent pci bridge is removed already? AFAICS this shouldn't break anything but people did seem to expect the removals to be ordered from child to parent. Bjorn, is this something you expect to happened? I do not expect a PCI bridge to be removed before the devices below it. We should be removing all the children before removing the parent bridge. But is this related to PCI? I don't see the connection yet. I tried I'm not sure. It was from thunderbolt and nobody is reporting it on other interconnects, so it could be. to look into this a bit (my notes are at https://bugzilla.kernel.org/show_bug.cgi?id=65281), but I haven't figured out the big-picture problem yet. I don't have warm fuzzies that adding a have we already removed this check is the best resolution, but maybe that's just because I don't understand the problem. Yeah, the whole thing is sorta pointless. Just issuing removal and continuing on should do, IMHO. I'd go for that as well. We have huge problems with the _del calls because visibility is strict hierarchy and it's not always easy to work out who's underneath us. It's going to be really annoying when refcounting works perfectly for objects, so you can just do puts in any order, but you have to have _del() called to remove subordinate objects before their parent. Well, that would be fine and dandy, but device_del() calls bus_remove_device() which in turn runs device_release_driver() and the order in which *these* things are done actually matters in general. So yes, after we have released all of the drivers in question, we can do _del() right before the final _put() in any order just fine. Thanks, Rafael -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
Rafael, As you pointed out, ata_tport_delete() should be after scsi_remove_host(), consistent with ata_tport_add() currently before ata_scsi_add_host(). Thanks for fixing it, Gwendal. On Mon, Nov 25, 2013 at 2:41 AM, Rafael J. Wysocki r...@rjwysocki.net wrote: On Monday, November 25, 2013 12:11:54 PM Mika Westerberg wrote: On Sun, Nov 24, 2013 at 02:09:09AM +0100, Rafael J. Wysocki wrote: On Sunday, November 24, 2013 12:36:03 AM Rafael J. Wysocki wrote: On Saturday, November 23, 2013 03:07:01 PM Greg Kroah-Hartman wrote: On Sun, Nov 24, 2013 at 12:12:59AM +0100, Rafael J. Wysocki wrote: On Saturday, November 23, 2013 02:53:58 PM Greg Kroah-Hartman wrote: [...] I can revert Mika's patch, as it would be good to catch these kinds of errors. Then we'll need to untangle the SATA/SCSI mess triggered by Bjorn in https://bugzilla.kernel.org/show_bug.cgi?id=65281. ;-) I have no objection to fixing that, the scsi sysfs handling is odd to say the least... If someone can unwind it, that would be great to see happen... Well, if I'm bored to death during the xmas holidays, I may look into that. In fact, I'm not exactly sure why ata_port_detach() calls ata_tport_delete() before scsi_remove_host()? Is there any particular reason? Because that doesn't seem to be exactly right ... I tried so that I have your 'PCI: Move device_del() from pci_stop_dev() to pci_destroy_dev()' applied and then I did following change as you suggested. diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 81a94a3919db..07a03f93d640 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -6304,10 +6304,10 @@ static void ata_port_detach(struct ata_port *ap) for (i = 0; i SATA_PMP_MAX_PORTS; i++) ata_tlink_delete(ap-pmp_link[i]); } - ata_tport_delete(ap); - /* remove the associated SCSI host */ scsi_remove_host(ap-scsi_host); + + ata_tport_delete(ap); } /** After both patches are applied the warnings are gone :) However, looks like both are needed since if I only apply one or another, I still get warnings. Yes, they are both needed. :-) OK, I'll submit the above upstream if you don't mind. Cheers, R. -- To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
(cc'ing Gwendal, hi!) On Sun, Nov 24, 2013 at 02:09:09AM +0100, Rafael J. Wysocki wrote: > On Sunday, November 24, 2013 12:36:03 AM Rafael J. Wysocki wrote: > > On Saturday, November 23, 2013 03:07:01 PM Greg Kroah-Hartman wrote: > > > On Sun, Nov 24, 2013 at 12:12:59AM +0100, Rafael J. Wysocki wrote: > > > > On Saturday, November 23, 2013 02:53:58 PM Greg Kroah-Hartman wrote: > > [...] > > > > > > > > > > > I can revert Mika's patch, as it would be good to catch these kinds of > > > > > errors. > > > > > > > > Then we'll need to untangle the SATA/SCSI mess triggered by Bjorn in > > > > https://bugzilla.kernel.org/show_bug.cgi?id=65281. ;-) > > > > > > I have no objection to fixing that, the scsi sysfs handling is "odd" to > > > say the least... > > > > > > If someone can unwind it, that would be great to see happen... > > > > Well, if I'm bored to death during the xmas holidays, I may look into that. > > In fact, I'm not exactly sure why ata_port_detach() calls ata_tport_delete() > before scsi_remove_host()? Is there any particular reason? Because that > doesn't seem to be exactly right ... No idea. Gwendal? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
(cc'ing Gwendal, hi!) On Sun, Nov 24, 2013 at 02:09:09AM +0100, Rafael J. Wysocki wrote: On Sunday, November 24, 2013 12:36:03 AM Rafael J. Wysocki wrote: On Saturday, November 23, 2013 03:07:01 PM Greg Kroah-Hartman wrote: On Sun, Nov 24, 2013 at 12:12:59AM +0100, Rafael J. Wysocki wrote: On Saturday, November 23, 2013 02:53:58 PM Greg Kroah-Hartman wrote: [...] I can revert Mika's patch, as it would be good to catch these kinds of errors. Then we'll need to untangle the SATA/SCSI mess triggered by Bjorn in https://bugzilla.kernel.org/show_bug.cgi?id=65281. ;-) I have no objection to fixing that, the scsi sysfs handling is odd to say the least... If someone can unwind it, that would be great to see happen... Well, if I'm bored to death during the xmas holidays, I may look into that. In fact, I'm not exactly sure why ata_port_detach() calls ata_tport_delete() before scsi_remove_host()? Is there any particular reason? Because that doesn't seem to be exactly right ... No idea. Gwendal? Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Sunday, November 24, 2013 12:36:03 AM Rafael J. Wysocki wrote: > On Saturday, November 23, 2013 03:07:01 PM Greg Kroah-Hartman wrote: > > On Sun, Nov 24, 2013 at 12:12:59AM +0100, Rafael J. Wysocki wrote: > > > On Saturday, November 23, 2013 02:53:58 PM Greg Kroah-Hartman wrote: [...] > > > > > > > > I can revert Mika's patch, as it would be good to catch these kinds of > > > > errors. > > > > > > Then we'll need to untangle the SATA/SCSI mess triggered by Bjorn in > > > https://bugzilla.kernel.org/show_bug.cgi?id=65281. ;-) > > > > I have no objection to fixing that, the scsi sysfs handling is "odd" to > > say the least... > > > > If someone can unwind it, that would be great to see happen... > > Well, if I'm bored to death during the xmas holidays, I may look into that. In fact, I'm not exactly sure why ata_port_detach() calls ata_tport_delete() before scsi_remove_host()? Is there any particular reason? Because that doesn't seem to be exactly right ... Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Saturday, November 23, 2013 03:07:01 PM Greg Kroah-Hartman wrote: > On Sun, Nov 24, 2013 at 12:12:59AM +0100, Rafael J. Wysocki wrote: > > On Saturday, November 23, 2013 02:53:58 PM Greg Kroah-Hartman wrote: > > > On Sat, Nov 23, 2013 at 11:56:48PM +0100, Rafael J. Wysocki wrote: > > > > On Friday, November 22, 2013 08:43:55 AM Bjorn Helgaas wrote: > > > > > [+cc Rafael, James] > > > > > > > > > > On Tue, Nov 19, 2013 at 11:18 PM, Tejun Heo wrote: > > > > > > (cc'ing Bjorn) > > > > > > > > > > > > Hello, > > > > > > > > > > > > On Tue, Nov 19, 2013 at 03:09:58PM +0200, Mika Westerberg wrote: > > > > > >> Commit bcdde7e221a8 (sysfs: make __sysfs_remove_dir() recursive) > > > > > >> changed > > > > > >> the behavior so that directory removals will be done recursively. > > > > > >> This > > > > > >> means that the sysfs group might already be removed if its parent > > > > > >> directory > > > > > >> has been removed. > > > > > >> > > > > > >> The current code outputs warnings similar to following log snippet > > > > > >> when it > > > > > >> detects that there is no group for the given kobject: > > > > > >> > > > > > >> WARNING: CPU: 0 PID: 4 at fs/sysfs/group.c:214 > > > > > >> sysfs_remove_group+0xc6/0xd0() > > > > > >> sysfs group 81c6f1e0 not found for kobject 'host7' > > > > > >> Modules linked in: > > > > > >> CPU: 0 PID: 4 Comm: kworker/0:0 Not tainted 3.12.0+ #13 > > > > > >> Hardware name: /D33217CK, BIOS > > > > > >> GKPPT10H.86A.0042.2013.0422.1439 04/22/2013 > > > > > >> Workqueue: kacpi_hotplug acpi_hotplug_work_fn > > > > > >> 0009 8801002459b0 817daab1 > > > > > >> 8801002459f8 > > > > > >> 8801002459e8 810436b8 > > > > > >> 81c6f1e0 > > > > > >> 88006d440358 88006d440188 88006e8b4c28 > > > > > >> 880100245a48 > > > > > >> Call Trace: > > > > > >> [] dump_stack+0x45/0x56 > > > > > >> [] warn_slowpath_common+0x78/0xa0 > > > > > >> [] warn_slowpath_fmt+0x47/0x50 > > > > > >> [] sysfs_remove_group+0xc6/0xd0 > > > > > >> [] dpm_sysfs_remove+0x3e/0x50 > > > > > >> [] device_del+0x40/0x1b0 > > > > > >> [] device_unregister+0xd/0x20 > > > > > >> [] scsi_remove_host+0xba/0x110 > > > > > >> [] ata_host_detach+0xc6/0x100 > > > > > >> [] ata_pci_remove_one+0x18/0x20 > > > > > >> [] pci_device_remove+0x28/0x60 > > > > > >> [] __device_release_driver+0x64/0xd0 > > > > > >> [] device_release_driver+0x1e/0x30 > > > > > >> [] bus_remove_device+0xf7/0x140 > > > > > >> [] device_del+0x121/0x1b0 > > > > > >> [] pci_stop_bus_device+0x94/0xa0 > > > > > >> [] pci_stop_bus_device+0x3b/0xa0 > > > > > >> [] pci_stop_bus_device+0x3b/0xa0 > > > > > >> [] pci_stop_and_remove_bus_device+0xd/0x20 > > > > > >> [] trim_stale_devices+0x73/0xe0 > > > > > >> [] trim_stale_devices+0xbb/0xe0 > > > > > >> [] trim_stale_devices+0xbb/0xe0 > > > > > >> [] acpiphp_check_bridge+0x7e/0xd0 > > > > > >> [] hotplug_event+0xcd/0x160 > > > > > >> [] hotplug_event_work+0x25/0x60 > > > > > >> [] acpi_hotplug_work_fn+0x17/0x22 > > > > > >> [] process_one_work+0x17a/0x430 > > > > > >> [] worker_thread+0x119/0x390 > > > > > >> [] kthread+0xcd/0xf0 > > > > > >> [] ret_from_fork+0x7c/0xb0 > > > > > > > > > > > > So, we do have cases where the parent is removed before the child. > > > > > > I > > > > > > suppose the parent pci bridge is removed already? AFAICS this > > > > > > shouldn't break anything but people did seem to expect the removals > > > > > > to > > > > > > be ordered from child to parent. Bjorn, is this something you > > > > > > expect > > > > > > to happened? > > > > > > > > > > I do not expect a PCI bridge to be removed before the devices below > > > > > it. We should be removing all the children before removing the parent > > > > > bridge. > > > > > > > > > > But is this related to PCI? I don't see the connection yet. I tried > > > > > to look into this a bit (my notes are at > > > > > https://bugzilla.kernel.org/show_bug.cgi?id=65281), but I haven't > > > > > figured out the big-picture problem yet. > > > > > > > > OK, so I think I've figured this out. > > > > > > > > As I wrote in the above BZ entry, after the Tejun's patch the > > > > device_del() in pci_stop_dev() removes the subordinate bus object's > > > > "power" directory among other things and because of that the warning > > > > triggers when we do the pci_remove_bus() in pci_remove_bus_device() > > > > for the same device. > > > > > > > > The appended patch fixes it for me, but Greg has already taken the > > > > Mika's one, so this one isn't really necessary. In case you think it > > > > would be useful to apply it anyway, please let me know and I submit it > > > > properly with a changelog etc. > > > > > > I can revert Mika's patch, as it would be good to catch these kinds of > > > errors. > > > > Then we'll need to untangle the SATA/SCSI mess triggered by Bjorn in
Re: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Sun, Nov 24, 2013 at 12:12:59AM +0100, Rafael J. Wysocki wrote: > On Saturday, November 23, 2013 02:53:58 PM Greg Kroah-Hartman wrote: > > On Sat, Nov 23, 2013 at 11:56:48PM +0100, Rafael J. Wysocki wrote: > > > On Friday, November 22, 2013 08:43:55 AM Bjorn Helgaas wrote: > > > > [+cc Rafael, James] > > > > > > > > On Tue, Nov 19, 2013 at 11:18 PM, Tejun Heo wrote: > > > > > (cc'ing Bjorn) > > > > > > > > > > Hello, > > > > > > > > > > On Tue, Nov 19, 2013 at 03:09:58PM +0200, Mika Westerberg wrote: > > > > >> Commit bcdde7e221a8 (sysfs: make __sysfs_remove_dir() recursive) > > > > >> changed > > > > >> the behavior so that directory removals will be done recursively. > > > > >> This > > > > >> means that the sysfs group might already be removed if its parent > > > > >> directory > > > > >> has been removed. > > > > >> > > > > >> The current code outputs warnings similar to following log snippet > > > > >> when it > > > > >> detects that there is no group for the given kobject: > > > > >> > > > > >> WARNING: CPU: 0 PID: 4 at fs/sysfs/group.c:214 > > > > >> sysfs_remove_group+0xc6/0xd0() > > > > >> sysfs group 81c6f1e0 not found for kobject 'host7' > > > > >> Modules linked in: > > > > >> CPU: 0 PID: 4 Comm: kworker/0:0 Not tainted 3.12.0+ #13 > > > > >> Hardware name: /D33217CK, BIOS > > > > >> GKPPT10H.86A.0042.2013.0422.1439 04/22/2013 > > > > >> Workqueue: kacpi_hotplug acpi_hotplug_work_fn > > > > >> 0009 8801002459b0 817daab1 8801002459f8 > > > > >> 8801002459e8 810436b8 81c6f1e0 > > > > >> 88006d440358 88006d440188 88006e8b4c28 880100245a48 > > > > >> Call Trace: > > > > >> [] dump_stack+0x45/0x56 > > > > >> [] warn_slowpath_common+0x78/0xa0 > > > > >> [] warn_slowpath_fmt+0x47/0x50 > > > > >> [] sysfs_remove_group+0xc6/0xd0 > > > > >> [] dpm_sysfs_remove+0x3e/0x50 > > > > >> [] device_del+0x40/0x1b0 > > > > >> [] device_unregister+0xd/0x20 > > > > >> [] scsi_remove_host+0xba/0x110 > > > > >> [] ata_host_detach+0xc6/0x100 > > > > >> [] ata_pci_remove_one+0x18/0x20 > > > > >> [] pci_device_remove+0x28/0x60 > > > > >> [] __device_release_driver+0x64/0xd0 > > > > >> [] device_release_driver+0x1e/0x30 > > > > >> [] bus_remove_device+0xf7/0x140 > > > > >> [] device_del+0x121/0x1b0 > > > > >> [] pci_stop_bus_device+0x94/0xa0 > > > > >> [] pci_stop_bus_device+0x3b/0xa0 > > > > >> [] pci_stop_bus_device+0x3b/0xa0 > > > > >> [] pci_stop_and_remove_bus_device+0xd/0x20 > > > > >> [] trim_stale_devices+0x73/0xe0 > > > > >> [] trim_stale_devices+0xbb/0xe0 > > > > >> [] trim_stale_devices+0xbb/0xe0 > > > > >> [] acpiphp_check_bridge+0x7e/0xd0 > > > > >> [] hotplug_event+0xcd/0x160 > > > > >> [] hotplug_event_work+0x25/0x60 > > > > >> [] acpi_hotplug_work_fn+0x17/0x22 > > > > >> [] process_one_work+0x17a/0x430 > > > > >> [] worker_thread+0x119/0x390 > > > > >> [] kthread+0xcd/0xf0 > > > > >> [] ret_from_fork+0x7c/0xb0 > > > > > > > > > > So, we do have cases where the parent is removed before the child. I > > > > > suppose the parent pci bridge is removed already? AFAICS this > > > > > shouldn't break anything but people did seem to expect the removals to > > > > > be ordered from child to parent. Bjorn, is this something you expect > > > > > to happened? > > > > > > > > I do not expect a PCI bridge to be removed before the devices below > > > > it. We should be removing all the children before removing the parent > > > > bridge. > > > > > > > > But is this related to PCI? I don't see the connection yet. I tried > > > > to look into this a bit (my notes are at > > > > https://bugzilla.kernel.org/show_bug.cgi?id=65281), but I haven't > > > > figured out the big-picture problem yet. > > > > > > OK, so I think I've figured this out. > > > > > > As I wrote in the above BZ entry, after the Tejun's patch the > > > device_del() in pci_stop_dev() removes the subordinate bus object's > > > "power" directory among other things and because of that the warning > > > triggers when we do the pci_remove_bus() in pci_remove_bus_device() > > > for the same device. > > > > > > The appended patch fixes it for me, but Greg has already taken the > > > Mika's one, so this one isn't really necessary. In case you think it > > > would be useful to apply it anyway, please let me know and I submit it > > > properly with a changelog etc. > > > > I can revert Mika's patch, as it would be good to catch these kinds of > > errors. > > Then we'll need to untangle the SATA/SCSI mess triggered by Bjorn in > https://bugzilla.kernel.org/show_bug.cgi?id=65281. ;-) I have no objection to fixing that, the scsi sysfs handling is "odd" to say the least... If someone can unwind it, that would be great to see happen... thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to
Re: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Saturday, November 23, 2013 02:53:58 PM Greg Kroah-Hartman wrote:
> On Sat, Nov 23, 2013 at 11:56:48PM +0100, Rafael J. Wysocki wrote:
> > On Friday, November 22, 2013 08:43:55 AM Bjorn Helgaas wrote:
> > > [+cc Rafael, James]
> > >
> > > On Tue, Nov 19, 2013 at 11:18 PM, Tejun Heo wrote:
> > > > (cc'ing Bjorn)
> > > >
> > > > Hello,
> > > >
> > > > On Tue, Nov 19, 2013 at 03:09:58PM +0200, Mika Westerberg wrote:
> > > >> Commit bcdde7e221a8 (sysfs: make __sysfs_remove_dir() recursive)
> > > >> changed
> > > >> the behavior so that directory removals will be done recursively. This
> > > >> means that the sysfs group might already be removed if its parent
> > > >> directory
> > > >> has been removed.
> > > >>
> > > >> The current code outputs warnings similar to following log snippet
> > > >> when it
> > > >> detects that there is no group for the given kobject:
> > > >>
> > > >> WARNING: CPU: 0 PID: 4 at fs/sysfs/group.c:214
> > > >> sysfs_remove_group+0xc6/0xd0()
> > > >> sysfs group 81c6f1e0 not found for kobject 'host7'
> > > >> Modules linked in:
> > > >> CPU: 0 PID: 4 Comm: kworker/0:0 Not tainted 3.12.0+ #13
> > > >> Hardware name: /D33217CK, BIOS
> > > >> GKPPT10H.86A.0042.2013.0422.1439 04/22/2013
> > > >> Workqueue: kacpi_hotplug acpi_hotplug_work_fn
> > > >> 0009 8801002459b0 817daab1 8801002459f8
> > > >> 8801002459e8 810436b8 81c6f1e0
> > > >> 88006d440358 88006d440188 88006e8b4c28 880100245a48
> > > >> Call Trace:
> > > >> [] dump_stack+0x45/0x56
> > > >> [] warn_slowpath_common+0x78/0xa0
> > > >> [] warn_slowpath_fmt+0x47/0x50
> > > >> [] sysfs_remove_group+0xc6/0xd0
> > > >> [] dpm_sysfs_remove+0x3e/0x50
> > > >> [] device_del+0x40/0x1b0
> > > >> [] device_unregister+0xd/0x20
> > > >> [] scsi_remove_host+0xba/0x110
> > > >> [] ata_host_detach+0xc6/0x100
> > > >> [] ata_pci_remove_one+0x18/0x20
> > > >> [] pci_device_remove+0x28/0x60
> > > >> [] __device_release_driver+0x64/0xd0
> > > >> [] device_release_driver+0x1e/0x30
> > > >> [] bus_remove_device+0xf7/0x140
> > > >> [] device_del+0x121/0x1b0
> > > >> [] pci_stop_bus_device+0x94/0xa0
> > > >> [] pci_stop_bus_device+0x3b/0xa0
> > > >> [] pci_stop_bus_device+0x3b/0xa0
> > > >> [] pci_stop_and_remove_bus_device+0xd/0x20
> > > >> [] trim_stale_devices+0x73/0xe0
> > > >> [] trim_stale_devices+0xbb/0xe0
> > > >> [] trim_stale_devices+0xbb/0xe0
> > > >> [] acpiphp_check_bridge+0x7e/0xd0
> > > >> [] hotplug_event+0xcd/0x160
> > > >> [] hotplug_event_work+0x25/0x60
> > > >> [] acpi_hotplug_work_fn+0x17/0x22
> > > >> [] process_one_work+0x17a/0x430
> > > >> [] worker_thread+0x119/0x390
> > > >> [] kthread+0xcd/0xf0
> > > >> [] ret_from_fork+0x7c/0xb0
> > > >
> > > > So, we do have cases where the parent is removed before the child. I
> > > > suppose the parent pci bridge is removed already? AFAICS this
> > > > shouldn't break anything but people did seem to expect the removals to
> > > > be ordered from child to parent. Bjorn, is this something you expect
> > > > to happened?
> > >
> > > I do not expect a PCI bridge to be removed before the devices below
> > > it. We should be removing all the children before removing the parent
> > > bridge.
> > >
> > > But is this related to PCI? I don't see the connection yet. I tried
> > > to look into this a bit (my notes are at
> > > https://bugzilla.kernel.org/show_bug.cgi?id=65281), but I haven't
> > > figured out the big-picture problem yet.
> >
> > OK, so I think I've figured this out.
> >
> > As I wrote in the above BZ entry, after the Tejun's patch the
> > device_del() in pci_stop_dev() removes the subordinate bus object's
> > "power" directory among other things and because of that the warning
> > triggers when we do the pci_remove_bus() in pci_remove_bus_device()
> > for the same device.
> >
> > The appended patch fixes it for me, but Greg has already taken the
> > Mika's one, so this one isn't really necessary. In case you think it
> > would be useful to apply it anyway, please let me know and I submit it
> > properly with a changelog etc.
>
> I can revert Mika's patch, as it would be good to catch these kinds of
> errors.
Then we'll need to untangle the SATA/SCSI mess triggered by Bjorn in
https://bugzilla.kernel.org/show_bug.cgi?id=65281. ;-)
> > ---
> > drivers/pci/remove.c |4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > Index: linux-pm/drivers/pci/remove.c
> > ===
> > --- linux-pm.orig/drivers/pci/remove.c
> > +++ linux-pm/drivers/pci/remove.c
> > @@ -24,7 +24,7 @@ static void pci_stop_dev(struct pci_dev
> > if (dev->is_added) {
> > pci_proc_detach_device(dev);
> > pci_remove_sysfs_dev_files(dev);
> > - device_del(>dev);
> > + device_release_driver(>dev);
> >
Re: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Sat, Nov 23, 2013 at 11:56:48PM +0100, Rafael J. Wysocki wrote:
> On Friday, November 22, 2013 08:43:55 AM Bjorn Helgaas wrote:
> > [+cc Rafael, James]
> >
> > On Tue, Nov 19, 2013 at 11:18 PM, Tejun Heo wrote:
> > > (cc'ing Bjorn)
> > >
> > > Hello,
> > >
> > > On Tue, Nov 19, 2013 at 03:09:58PM +0200, Mika Westerberg wrote:
> > >> Commit bcdde7e221a8 (sysfs: make __sysfs_remove_dir() recursive) changed
> > >> the behavior so that directory removals will be done recursively. This
> > >> means that the sysfs group might already be removed if its parent
> > >> directory
> > >> has been removed.
> > >>
> > >> The current code outputs warnings similar to following log snippet when
> > >> it
> > >> detects that there is no group for the given kobject:
> > >>
> > >> WARNING: CPU: 0 PID: 4 at fs/sysfs/group.c:214
> > >> sysfs_remove_group+0xc6/0xd0()
> > >> sysfs group 81c6f1e0 not found for kobject 'host7'
> > >> Modules linked in:
> > >> CPU: 0 PID: 4 Comm: kworker/0:0 Not tainted 3.12.0+ #13
> > >> Hardware name: /D33217CK, BIOS
> > >> GKPPT10H.86A.0042.2013.0422.1439 04/22/2013
> > >> Workqueue: kacpi_hotplug acpi_hotplug_work_fn
> > >> 0009 8801002459b0 817daab1 8801002459f8
> > >> 8801002459e8 810436b8 81c6f1e0
> > >> 88006d440358 88006d440188 88006e8b4c28 880100245a48
> > >> Call Trace:
> > >> [] dump_stack+0x45/0x56
> > >> [] warn_slowpath_common+0x78/0xa0
> > >> [] warn_slowpath_fmt+0x47/0x50
> > >> [] sysfs_remove_group+0xc6/0xd0
> > >> [] dpm_sysfs_remove+0x3e/0x50
> > >> [] device_del+0x40/0x1b0
> > >> [] device_unregister+0xd/0x20
> > >> [] scsi_remove_host+0xba/0x110
> > >> [] ata_host_detach+0xc6/0x100
> > >> [] ata_pci_remove_one+0x18/0x20
> > >> [] pci_device_remove+0x28/0x60
> > >> [] __device_release_driver+0x64/0xd0
> > >> [] device_release_driver+0x1e/0x30
> > >> [] bus_remove_device+0xf7/0x140
> > >> [] device_del+0x121/0x1b0
> > >> [] pci_stop_bus_device+0x94/0xa0
> > >> [] pci_stop_bus_device+0x3b/0xa0
> > >> [] pci_stop_bus_device+0x3b/0xa0
> > >> [] pci_stop_and_remove_bus_device+0xd/0x20
> > >> [] trim_stale_devices+0x73/0xe0
> > >> [] trim_stale_devices+0xbb/0xe0
> > >> [] trim_stale_devices+0xbb/0xe0
> > >> [] acpiphp_check_bridge+0x7e/0xd0
> > >> [] hotplug_event+0xcd/0x160
> > >> [] hotplug_event_work+0x25/0x60
> > >> [] acpi_hotplug_work_fn+0x17/0x22
> > >> [] process_one_work+0x17a/0x430
> > >> [] worker_thread+0x119/0x390
> > >> [] kthread+0xcd/0xf0
> > >> [] ret_from_fork+0x7c/0xb0
> > >
> > > So, we do have cases where the parent is removed before the child. I
> > > suppose the parent pci bridge is removed already? AFAICS this
> > > shouldn't break anything but people did seem to expect the removals to
> > > be ordered from child to parent. Bjorn, is this something you expect
> > > to happened?
> >
> > I do not expect a PCI bridge to be removed before the devices below
> > it. We should be removing all the children before removing the parent
> > bridge.
> >
> > But is this related to PCI? I don't see the connection yet. I tried
> > to look into this a bit (my notes are at
> > https://bugzilla.kernel.org/show_bug.cgi?id=65281), but I haven't
> > figured out the big-picture problem yet.
>
> OK, so I think I've figured this out.
>
> As I wrote in the above BZ entry, after the Tejun's patch the
> device_del() in pci_stop_dev() removes the subordinate bus object's
> "power" directory among other things and because of that the warning
> triggers when we do the pci_remove_bus() in pci_remove_bus_device()
> for the same device.
>
> The appended patch fixes it for me, but Greg has already taken the
> Mika's one, so this one isn't really necessary. In case you think it
> would be useful to apply it anyway, please let me know and I submit it
> properly with a changelog etc.
I can revert Mika's patch, as it would be good to catch these kinds of
errors.
> ---
> drivers/pci/remove.c |4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> Index: linux-pm/drivers/pci/remove.c
> ===
> --- linux-pm.orig/drivers/pci/remove.c
> +++ linux-pm/drivers/pci/remove.c
> @@ -24,7 +24,7 @@ static void pci_stop_dev(struct pci_dev
> if (dev->is_added) {
> pci_proc_detach_device(dev);
> pci_remove_sysfs_dev_files(dev);
> - device_del(>dev);
> + device_release_driver(>dev);
> dev->is_added = 0;
> }
>
> @@ -34,6 +34,8 @@ static void pci_stop_dev(struct pci_dev
>
> static void pci_destroy_dev(struct pci_dev *dev)
> {
> + device_del(>dev);
> +
> down_write(_bus_sem);
> list_del(>bus_list);
> up_write(_bus_sem);
This looks good, thanks for finding this. I suggest resending it with a
changelog so that Bjorn can get it into 3.13.
Re: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Friday, November 22, 2013 08:43:55 AM Bjorn Helgaas wrote:
> [+cc Rafael, James]
>
> On Tue, Nov 19, 2013 at 11:18 PM, Tejun Heo wrote:
> > (cc'ing Bjorn)
> >
> > Hello,
> >
> > On Tue, Nov 19, 2013 at 03:09:58PM +0200, Mika Westerberg wrote:
> >> Commit bcdde7e221a8 (sysfs: make __sysfs_remove_dir() recursive) changed
> >> the behavior so that directory removals will be done recursively. This
> >> means that the sysfs group might already be removed if its parent directory
> >> has been removed.
> >>
> >> The current code outputs warnings similar to following log snippet when it
> >> detects that there is no group for the given kobject:
> >>
> >> WARNING: CPU: 0 PID: 4 at fs/sysfs/group.c:214
> >> sysfs_remove_group+0xc6/0xd0()
> >> sysfs group 81c6f1e0 not found for kobject 'host7'
> >> Modules linked in:
> >> CPU: 0 PID: 4 Comm: kworker/0:0 Not tainted 3.12.0+ #13
> >> Hardware name: /D33217CK, BIOS
> >> GKPPT10H.86A.0042.2013.0422.1439 04/22/2013
> >> Workqueue: kacpi_hotplug acpi_hotplug_work_fn
> >> 0009 8801002459b0 817daab1 8801002459f8
> >> 8801002459e8 810436b8 81c6f1e0
> >> 88006d440358 88006d440188 88006e8b4c28 880100245a48
> >> Call Trace:
> >> [] dump_stack+0x45/0x56
> >> [] warn_slowpath_common+0x78/0xa0
> >> [] warn_slowpath_fmt+0x47/0x50
> >> [] sysfs_remove_group+0xc6/0xd0
> >> [] dpm_sysfs_remove+0x3e/0x50
> >> [] device_del+0x40/0x1b0
> >> [] device_unregister+0xd/0x20
> >> [] scsi_remove_host+0xba/0x110
> >> [] ata_host_detach+0xc6/0x100
> >> [] ata_pci_remove_one+0x18/0x20
> >> [] pci_device_remove+0x28/0x60
> >> [] __device_release_driver+0x64/0xd0
> >> [] device_release_driver+0x1e/0x30
> >> [] bus_remove_device+0xf7/0x140
> >> [] device_del+0x121/0x1b0
> >> [] pci_stop_bus_device+0x94/0xa0
> >> [] pci_stop_bus_device+0x3b/0xa0
> >> [] pci_stop_bus_device+0x3b/0xa0
> >> [] pci_stop_and_remove_bus_device+0xd/0x20
> >> [] trim_stale_devices+0x73/0xe0
> >> [] trim_stale_devices+0xbb/0xe0
> >> [] trim_stale_devices+0xbb/0xe0
> >> [] acpiphp_check_bridge+0x7e/0xd0
> >> [] hotplug_event+0xcd/0x160
> >> [] hotplug_event_work+0x25/0x60
> >> [] acpi_hotplug_work_fn+0x17/0x22
> >> [] process_one_work+0x17a/0x430
> >> [] worker_thread+0x119/0x390
> >> [] kthread+0xcd/0xf0
> >> [] ret_from_fork+0x7c/0xb0
> >
> > So, we do have cases where the parent is removed before the child. I
> > suppose the parent pci bridge is removed already? AFAICS this
> > shouldn't break anything but people did seem to expect the removals to
> > be ordered from child to parent. Bjorn, is this something you expect
> > to happened?
>
> I do not expect a PCI bridge to be removed before the devices below
> it. We should be removing all the children before removing the parent
> bridge.
>
> But is this related to PCI? I don't see the connection yet. I tried
> to look into this a bit (my notes are at
> https://bugzilla.kernel.org/show_bug.cgi?id=65281), but I haven't
> figured out the big-picture problem yet.
OK, so I think I've figured this out.
As I wrote in the above BZ entry, after the Tejun's patch the
device_del() in pci_stop_dev() removes the subordinate bus object's
"power" directory among other things and because of that the warning
triggers when we do the pci_remove_bus() in pci_remove_bus_device()
for the same device.
The appended patch fixes it for me, but Greg has already taken the
Mika's one, so this one isn't really necessary. In case you think it
would be useful to apply it anyway, please let me know and I submit it
properly with a changelog etc.
Thanks,
Rafael
---
drivers/pci/remove.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Index: linux-pm/drivers/pci/remove.c
===
--- linux-pm.orig/drivers/pci/remove.c
+++ linux-pm/drivers/pci/remove.c
@@ -24,7 +24,7 @@ static void pci_stop_dev(struct pci_dev
if (dev->is_added) {
pci_proc_detach_device(dev);
pci_remove_sysfs_dev_files(dev);
- device_del(>dev);
+ device_release_driver(>dev);
dev->is_added = 0;
}
@@ -34,6 +34,8 @@ static void pci_stop_dev(struct pci_dev
static void pci_destroy_dev(struct pci_dev *dev)
{
+ device_del(>dev);
+
down_write(_bus_sem);
list_del(>bus_list);
up_write(_bus_sem);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Friday, November 22, 2013 08:43:55 AM Bjorn Helgaas wrote:
[+cc Rafael, James]
On Tue, Nov 19, 2013 at 11:18 PM, Tejun Heo t...@kernel.org wrote:
(cc'ing Bjorn)
Hello,
On Tue, Nov 19, 2013 at 03:09:58PM +0200, Mika Westerberg wrote:
Commit bcdde7e221a8 (sysfs: make __sysfs_remove_dir() recursive) changed
the behavior so that directory removals will be done recursively. This
means that the sysfs group might already be removed if its parent directory
has been removed.
The current code outputs warnings similar to following log snippet when it
detects that there is no group for the given kobject:
WARNING: CPU: 0 PID: 4 at fs/sysfs/group.c:214
sysfs_remove_group+0xc6/0xd0()
sysfs group 81c6f1e0 not found for kobject 'host7'
Modules linked in:
CPU: 0 PID: 4 Comm: kworker/0:0 Not tainted 3.12.0+ #13
Hardware name: /D33217CK, BIOS
GKPPT10H.86A.0042.2013.0422.1439 04/22/2013
Workqueue: kacpi_hotplug acpi_hotplug_work_fn
0009 8801002459b0 817daab1 8801002459f8
8801002459e8 810436b8 81c6f1e0
88006d440358 88006d440188 88006e8b4c28 880100245a48
Call Trace:
[817daab1] dump_stack+0x45/0x56
[810436b8] warn_slowpath_common+0x78/0xa0
[81043727] warn_slowpath_fmt+0x47/0x50
[811ae526] sysfs_remove_group+0xc6/0xd0
[81432f7e] dpm_sysfs_remove+0x3e/0x50
[8142a0d0] device_del+0x40/0x1b0
[8142a24d] device_unregister+0xd/0x20
[8144131a] scsi_remove_host+0xba/0x110
[8145f526] ata_host_detach+0xc6/0x100
[8145f578] ata_pci_remove_one+0x18/0x20
[812e8f48] pci_device_remove+0x28/0x60
[8142d854] __device_release_driver+0x64/0xd0
[8142d8de] device_release_driver+0x1e/0x30
[8142d257] bus_remove_device+0xf7/0x140
[8142a1b1] device_del+0x121/0x1b0
[812e43d4] pci_stop_bus_device+0x94/0xa0
[812e437b] pci_stop_bus_device+0x3b/0xa0
[812e437b] pci_stop_bus_device+0x3b/0xa0
[812e44dd] pci_stop_and_remove_bus_device+0xd/0x20
[812fc743] trim_stale_devices+0x73/0xe0
[812fc78b] trim_stale_devices+0xbb/0xe0
[812fc78b] trim_stale_devices+0xbb/0xe0
[812fcb6e] acpiphp_check_bridge+0x7e/0xd0
[812fd90d] hotplug_event+0xcd/0x160
[812fd9c5] hotplug_event_work+0x25/0x60
[81316749] acpi_hotplug_work_fn+0x17/0x22
[8105cf3a] process_one_work+0x17a/0x430
[8105db29] worker_thread+0x119/0x390
[81063a5d] kthread+0xcd/0xf0
[817eb33c] ret_from_fork+0x7c/0xb0
So, we do have cases where the parent is removed before the child. I
suppose the parent pci bridge is removed already? AFAICS this
shouldn't break anything but people did seem to expect the removals to
be ordered from child to parent. Bjorn, is this something you expect
to happened?
I do not expect a PCI bridge to be removed before the devices below
it. We should be removing all the children before removing the parent
bridge.
But is this related to PCI? I don't see the connection yet. I tried
to look into this a bit (my notes are at
https://bugzilla.kernel.org/show_bug.cgi?id=65281), but I haven't
figured out the big-picture problem yet.
OK, so I think I've figured this out.
As I wrote in the above BZ entry, after the Tejun's patch the
device_del() in pci_stop_dev() removes the subordinate bus object's
power directory among other things and because of that the warning
triggers when we do the pci_remove_bus() in pci_remove_bus_device()
for the same device.
The appended patch fixes it for me, but Greg has already taken the
Mika's one, so this one isn't really necessary. In case you think it
would be useful to apply it anyway, please let me know and I submit it
properly with a changelog etc.
Thanks,
Rafael
---
drivers/pci/remove.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Index: linux-pm/drivers/pci/remove.c
===
--- linux-pm.orig/drivers/pci/remove.c
+++ linux-pm/drivers/pci/remove.c
@@ -24,7 +24,7 @@ static void pci_stop_dev(struct pci_dev
if (dev-is_added) {
pci_proc_detach_device(dev);
pci_remove_sysfs_dev_files(dev);
- device_del(dev-dev);
+ device_release_driver(dev-dev);
dev-is_added = 0;
}
@@ -34,6 +34,8 @@ static void pci_stop_dev(struct pci_dev
static void pci_destroy_dev(struct pci_dev *dev)
{
+ device_del(dev-dev);
+
down_write(pci_bus_sem);
list_del(dev-bus_list);
up_write(pci_bus_sem);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at
Re: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Sat, Nov 23, 2013 at 11:56:48PM +0100, Rafael J. Wysocki wrote:
On Friday, November 22, 2013 08:43:55 AM Bjorn Helgaas wrote:
[+cc Rafael, James]
On Tue, Nov 19, 2013 at 11:18 PM, Tejun Heo t...@kernel.org wrote:
(cc'ing Bjorn)
Hello,
On Tue, Nov 19, 2013 at 03:09:58PM +0200, Mika Westerberg wrote:
Commit bcdde7e221a8 (sysfs: make __sysfs_remove_dir() recursive) changed
the behavior so that directory removals will be done recursively. This
means that the sysfs group might already be removed if its parent
directory
has been removed.
The current code outputs warnings similar to following log snippet when
it
detects that there is no group for the given kobject:
WARNING: CPU: 0 PID: 4 at fs/sysfs/group.c:214
sysfs_remove_group+0xc6/0xd0()
sysfs group 81c6f1e0 not found for kobject 'host7'
Modules linked in:
CPU: 0 PID: 4 Comm: kworker/0:0 Not tainted 3.12.0+ #13
Hardware name: /D33217CK, BIOS
GKPPT10H.86A.0042.2013.0422.1439 04/22/2013
Workqueue: kacpi_hotplug acpi_hotplug_work_fn
0009 8801002459b0 817daab1 8801002459f8
8801002459e8 810436b8 81c6f1e0
88006d440358 88006d440188 88006e8b4c28 880100245a48
Call Trace:
[817daab1] dump_stack+0x45/0x56
[810436b8] warn_slowpath_common+0x78/0xa0
[81043727] warn_slowpath_fmt+0x47/0x50
[811ae526] sysfs_remove_group+0xc6/0xd0
[81432f7e] dpm_sysfs_remove+0x3e/0x50
[8142a0d0] device_del+0x40/0x1b0
[8142a24d] device_unregister+0xd/0x20
[8144131a] scsi_remove_host+0xba/0x110
[8145f526] ata_host_detach+0xc6/0x100
[8145f578] ata_pci_remove_one+0x18/0x20
[812e8f48] pci_device_remove+0x28/0x60
[8142d854] __device_release_driver+0x64/0xd0
[8142d8de] device_release_driver+0x1e/0x30
[8142d257] bus_remove_device+0xf7/0x140
[8142a1b1] device_del+0x121/0x1b0
[812e43d4] pci_stop_bus_device+0x94/0xa0
[812e437b] pci_stop_bus_device+0x3b/0xa0
[812e437b] pci_stop_bus_device+0x3b/0xa0
[812e44dd] pci_stop_and_remove_bus_device+0xd/0x20
[812fc743] trim_stale_devices+0x73/0xe0
[812fc78b] trim_stale_devices+0xbb/0xe0
[812fc78b] trim_stale_devices+0xbb/0xe0
[812fcb6e] acpiphp_check_bridge+0x7e/0xd0
[812fd90d] hotplug_event+0xcd/0x160
[812fd9c5] hotplug_event_work+0x25/0x60
[81316749] acpi_hotplug_work_fn+0x17/0x22
[8105cf3a] process_one_work+0x17a/0x430
[8105db29] worker_thread+0x119/0x390
[81063a5d] kthread+0xcd/0xf0
[817eb33c] ret_from_fork+0x7c/0xb0
So, we do have cases where the parent is removed before the child. I
suppose the parent pci bridge is removed already? AFAICS this
shouldn't break anything but people did seem to expect the removals to
be ordered from child to parent. Bjorn, is this something you expect
to happened?
I do not expect a PCI bridge to be removed before the devices below
it. We should be removing all the children before removing the parent
bridge.
But is this related to PCI? I don't see the connection yet. I tried
to look into this a bit (my notes are at
https://bugzilla.kernel.org/show_bug.cgi?id=65281), but I haven't
figured out the big-picture problem yet.
OK, so I think I've figured this out.
As I wrote in the above BZ entry, after the Tejun's patch the
device_del() in pci_stop_dev() removes the subordinate bus object's
power directory among other things and because of that the warning
triggers when we do the pci_remove_bus() in pci_remove_bus_device()
for the same device.
The appended patch fixes it for me, but Greg has already taken the
Mika's one, so this one isn't really necessary. In case you think it
would be useful to apply it anyway, please let me know and I submit it
properly with a changelog etc.
I can revert Mika's patch, as it would be good to catch these kinds of
errors.
---
drivers/pci/remove.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Index: linux-pm/drivers/pci/remove.c
===
--- linux-pm.orig/drivers/pci/remove.c
+++ linux-pm/drivers/pci/remove.c
@@ -24,7 +24,7 @@ static void pci_stop_dev(struct pci_dev
if (dev-is_added) {
pci_proc_detach_device(dev);
pci_remove_sysfs_dev_files(dev);
- device_del(dev-dev);
+ device_release_driver(dev-dev);
dev-is_added = 0;
}
@@ -34,6 +34,8 @@ static void pci_stop_dev(struct pci_dev
static void pci_destroy_dev(struct pci_dev *dev)
{
+ device_del(dev-dev);
+
Re: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Saturday, November 23, 2013 02:53:58 PM Greg Kroah-Hartman wrote:
On Sat, Nov 23, 2013 at 11:56:48PM +0100, Rafael J. Wysocki wrote:
On Friday, November 22, 2013 08:43:55 AM Bjorn Helgaas wrote:
[+cc Rafael, James]
On Tue, Nov 19, 2013 at 11:18 PM, Tejun Heo t...@kernel.org wrote:
(cc'ing Bjorn)
Hello,
On Tue, Nov 19, 2013 at 03:09:58PM +0200, Mika Westerberg wrote:
Commit bcdde7e221a8 (sysfs: make __sysfs_remove_dir() recursive)
changed
the behavior so that directory removals will be done recursively. This
means that the sysfs group might already be removed if its parent
directory
has been removed.
The current code outputs warnings similar to following log snippet
when it
detects that there is no group for the given kobject:
WARNING: CPU: 0 PID: 4 at fs/sysfs/group.c:214
sysfs_remove_group+0xc6/0xd0()
sysfs group 81c6f1e0 not found for kobject 'host7'
Modules linked in:
CPU: 0 PID: 4 Comm: kworker/0:0 Not tainted 3.12.0+ #13
Hardware name: /D33217CK, BIOS
GKPPT10H.86A.0042.2013.0422.1439 04/22/2013
Workqueue: kacpi_hotplug acpi_hotplug_work_fn
0009 8801002459b0 817daab1 8801002459f8
8801002459e8 810436b8 81c6f1e0
88006d440358 88006d440188 88006e8b4c28 880100245a48
Call Trace:
[817daab1] dump_stack+0x45/0x56
[810436b8] warn_slowpath_common+0x78/0xa0
[81043727] warn_slowpath_fmt+0x47/0x50
[811ae526] sysfs_remove_group+0xc6/0xd0
[81432f7e] dpm_sysfs_remove+0x3e/0x50
[8142a0d0] device_del+0x40/0x1b0
[8142a24d] device_unregister+0xd/0x20
[8144131a] scsi_remove_host+0xba/0x110
[8145f526] ata_host_detach+0xc6/0x100
[8145f578] ata_pci_remove_one+0x18/0x20
[812e8f48] pci_device_remove+0x28/0x60
[8142d854] __device_release_driver+0x64/0xd0
[8142d8de] device_release_driver+0x1e/0x30
[8142d257] bus_remove_device+0xf7/0x140
[8142a1b1] device_del+0x121/0x1b0
[812e43d4] pci_stop_bus_device+0x94/0xa0
[812e437b] pci_stop_bus_device+0x3b/0xa0
[812e437b] pci_stop_bus_device+0x3b/0xa0
[812e44dd] pci_stop_and_remove_bus_device+0xd/0x20
[812fc743] trim_stale_devices+0x73/0xe0
[812fc78b] trim_stale_devices+0xbb/0xe0
[812fc78b] trim_stale_devices+0xbb/0xe0
[812fcb6e] acpiphp_check_bridge+0x7e/0xd0
[812fd90d] hotplug_event+0xcd/0x160
[812fd9c5] hotplug_event_work+0x25/0x60
[81316749] acpi_hotplug_work_fn+0x17/0x22
[8105cf3a] process_one_work+0x17a/0x430
[8105db29] worker_thread+0x119/0x390
[81063a5d] kthread+0xcd/0xf0
[817eb33c] ret_from_fork+0x7c/0xb0
So, we do have cases where the parent is removed before the child. I
suppose the parent pci bridge is removed already? AFAICS this
shouldn't break anything but people did seem to expect the removals to
be ordered from child to parent. Bjorn, is this something you expect
to happened?
I do not expect a PCI bridge to be removed before the devices below
it. We should be removing all the children before removing the parent
bridge.
But is this related to PCI? I don't see the connection yet. I tried
to look into this a bit (my notes are at
https://bugzilla.kernel.org/show_bug.cgi?id=65281), but I haven't
figured out the big-picture problem yet.
OK, so I think I've figured this out.
As I wrote in the above BZ entry, after the Tejun's patch the
device_del() in pci_stop_dev() removes the subordinate bus object's
power directory among other things and because of that the warning
triggers when we do the pci_remove_bus() in pci_remove_bus_device()
for the same device.
The appended patch fixes it for me, but Greg has already taken the
Mika's one, so this one isn't really necessary. In case you think it
would be useful to apply it anyway, please let me know and I submit it
properly with a changelog etc.
I can revert Mika's patch, as it would be good to catch these kinds of
errors.
Then we'll need to untangle the SATA/SCSI mess triggered by Bjorn in
https://bugzilla.kernel.org/show_bug.cgi?id=65281. ;-)
---
drivers/pci/remove.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Index: linux-pm/drivers/pci/remove.c
===
--- linux-pm.orig/drivers/pci/remove.c
+++ linux-pm/drivers/pci/remove.c
@@ -24,7 +24,7 @@ static void pci_stop_dev(struct pci_dev
if (dev-is_added) {
pci_proc_detach_device(dev);
Re: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Sun, Nov 24, 2013 at 12:12:59AM +0100, Rafael J. Wysocki wrote: On Saturday, November 23, 2013 02:53:58 PM Greg Kroah-Hartman wrote: On Sat, Nov 23, 2013 at 11:56:48PM +0100, Rafael J. Wysocki wrote: On Friday, November 22, 2013 08:43:55 AM Bjorn Helgaas wrote: [+cc Rafael, James] On Tue, Nov 19, 2013 at 11:18 PM, Tejun Heo t...@kernel.org wrote: (cc'ing Bjorn) Hello, On Tue, Nov 19, 2013 at 03:09:58PM +0200, Mika Westerberg wrote: Commit bcdde7e221a8 (sysfs: make __sysfs_remove_dir() recursive) changed the behavior so that directory removals will be done recursively. This means that the sysfs group might already be removed if its parent directory has been removed. The current code outputs warnings similar to following log snippet when it detects that there is no group for the given kobject: WARNING: CPU: 0 PID: 4 at fs/sysfs/group.c:214 sysfs_remove_group+0xc6/0xd0() sysfs group 81c6f1e0 not found for kobject 'host7' Modules linked in: CPU: 0 PID: 4 Comm: kworker/0:0 Not tainted 3.12.0+ #13 Hardware name: /D33217CK, BIOS GKPPT10H.86A.0042.2013.0422.1439 04/22/2013 Workqueue: kacpi_hotplug acpi_hotplug_work_fn 0009 8801002459b0 817daab1 8801002459f8 8801002459e8 810436b8 81c6f1e0 88006d440358 88006d440188 88006e8b4c28 880100245a48 Call Trace: [817daab1] dump_stack+0x45/0x56 [810436b8] warn_slowpath_common+0x78/0xa0 [81043727] warn_slowpath_fmt+0x47/0x50 [811ae526] sysfs_remove_group+0xc6/0xd0 [81432f7e] dpm_sysfs_remove+0x3e/0x50 [8142a0d0] device_del+0x40/0x1b0 [8142a24d] device_unregister+0xd/0x20 [8144131a] scsi_remove_host+0xba/0x110 [8145f526] ata_host_detach+0xc6/0x100 [8145f578] ata_pci_remove_one+0x18/0x20 [812e8f48] pci_device_remove+0x28/0x60 [8142d854] __device_release_driver+0x64/0xd0 [8142d8de] device_release_driver+0x1e/0x30 [8142d257] bus_remove_device+0xf7/0x140 [8142a1b1] device_del+0x121/0x1b0 [812e43d4] pci_stop_bus_device+0x94/0xa0 [812e437b] pci_stop_bus_device+0x3b/0xa0 [812e437b] pci_stop_bus_device+0x3b/0xa0 [812e44dd] pci_stop_and_remove_bus_device+0xd/0x20 [812fc743] trim_stale_devices+0x73/0xe0 [812fc78b] trim_stale_devices+0xbb/0xe0 [812fc78b] trim_stale_devices+0xbb/0xe0 [812fcb6e] acpiphp_check_bridge+0x7e/0xd0 [812fd90d] hotplug_event+0xcd/0x160 [812fd9c5] hotplug_event_work+0x25/0x60 [81316749] acpi_hotplug_work_fn+0x17/0x22 [8105cf3a] process_one_work+0x17a/0x430 [8105db29] worker_thread+0x119/0x390 [81063a5d] kthread+0xcd/0xf0 [817eb33c] ret_from_fork+0x7c/0xb0 So, we do have cases where the parent is removed before the child. I suppose the parent pci bridge is removed already? AFAICS this shouldn't break anything but people did seem to expect the removals to be ordered from child to parent. Bjorn, is this something you expect to happened? I do not expect a PCI bridge to be removed before the devices below it. We should be removing all the children before removing the parent bridge. But is this related to PCI? I don't see the connection yet. I tried to look into this a bit (my notes are at https://bugzilla.kernel.org/show_bug.cgi?id=65281), but I haven't figured out the big-picture problem yet. OK, so I think I've figured this out. As I wrote in the above BZ entry, after the Tejun's patch the device_del() in pci_stop_dev() removes the subordinate bus object's power directory among other things and because of that the warning triggers when we do the pci_remove_bus() in pci_remove_bus_device() for the same device. The appended patch fixes it for me, but Greg has already taken the Mika's one, so this one isn't really necessary. In case you think it would be useful to apply it anyway, please let me know and I submit it properly with a changelog etc. I can revert Mika's patch, as it would be good to catch these kinds of errors. Then we'll need to untangle the SATA/SCSI mess triggered by Bjorn in https://bugzilla.kernel.org/show_bug.cgi?id=65281. ;-) I have no objection to fixing that, the scsi sysfs handling is odd to say the least... If someone can unwind it, that would be great to see happen... thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to
Re: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Saturday, November 23, 2013 03:07:01 PM Greg Kroah-Hartman wrote: On Sun, Nov 24, 2013 at 12:12:59AM +0100, Rafael J. Wysocki wrote: On Saturday, November 23, 2013 02:53:58 PM Greg Kroah-Hartman wrote: On Sat, Nov 23, 2013 at 11:56:48PM +0100, Rafael J. Wysocki wrote: On Friday, November 22, 2013 08:43:55 AM Bjorn Helgaas wrote: [+cc Rafael, James] On Tue, Nov 19, 2013 at 11:18 PM, Tejun Heo t...@kernel.org wrote: (cc'ing Bjorn) Hello, On Tue, Nov 19, 2013 at 03:09:58PM +0200, Mika Westerberg wrote: Commit bcdde7e221a8 (sysfs: make __sysfs_remove_dir() recursive) changed the behavior so that directory removals will be done recursively. This means that the sysfs group might already be removed if its parent directory has been removed. The current code outputs warnings similar to following log snippet when it detects that there is no group for the given kobject: WARNING: CPU: 0 PID: 4 at fs/sysfs/group.c:214 sysfs_remove_group+0xc6/0xd0() sysfs group 81c6f1e0 not found for kobject 'host7' Modules linked in: CPU: 0 PID: 4 Comm: kworker/0:0 Not tainted 3.12.0+ #13 Hardware name: /D33217CK, BIOS GKPPT10H.86A.0042.2013.0422.1439 04/22/2013 Workqueue: kacpi_hotplug acpi_hotplug_work_fn 0009 8801002459b0 817daab1 8801002459f8 8801002459e8 810436b8 81c6f1e0 88006d440358 88006d440188 88006e8b4c28 880100245a48 Call Trace: [817daab1] dump_stack+0x45/0x56 [810436b8] warn_slowpath_common+0x78/0xa0 [81043727] warn_slowpath_fmt+0x47/0x50 [811ae526] sysfs_remove_group+0xc6/0xd0 [81432f7e] dpm_sysfs_remove+0x3e/0x50 [8142a0d0] device_del+0x40/0x1b0 [8142a24d] device_unregister+0xd/0x20 [8144131a] scsi_remove_host+0xba/0x110 [8145f526] ata_host_detach+0xc6/0x100 [8145f578] ata_pci_remove_one+0x18/0x20 [812e8f48] pci_device_remove+0x28/0x60 [8142d854] __device_release_driver+0x64/0xd0 [8142d8de] device_release_driver+0x1e/0x30 [8142d257] bus_remove_device+0xf7/0x140 [8142a1b1] device_del+0x121/0x1b0 [812e43d4] pci_stop_bus_device+0x94/0xa0 [812e437b] pci_stop_bus_device+0x3b/0xa0 [812e437b] pci_stop_bus_device+0x3b/0xa0 [812e44dd] pci_stop_and_remove_bus_device+0xd/0x20 [812fc743] trim_stale_devices+0x73/0xe0 [812fc78b] trim_stale_devices+0xbb/0xe0 [812fc78b] trim_stale_devices+0xbb/0xe0 [812fcb6e] acpiphp_check_bridge+0x7e/0xd0 [812fd90d] hotplug_event+0xcd/0x160 [812fd9c5] hotplug_event_work+0x25/0x60 [81316749] acpi_hotplug_work_fn+0x17/0x22 [8105cf3a] process_one_work+0x17a/0x430 [8105db29] worker_thread+0x119/0x390 [81063a5d] kthread+0xcd/0xf0 [817eb33c] ret_from_fork+0x7c/0xb0 So, we do have cases where the parent is removed before the child. I suppose the parent pci bridge is removed already? AFAICS this shouldn't break anything but people did seem to expect the removals to be ordered from child to parent. Bjorn, is this something you expect to happened? I do not expect a PCI bridge to be removed before the devices below it. We should be removing all the children before removing the parent bridge. But is this related to PCI? I don't see the connection yet. I tried to look into this a bit (my notes are at https://bugzilla.kernel.org/show_bug.cgi?id=65281), but I haven't figured out the big-picture problem yet. OK, so I think I've figured this out. As I wrote in the above BZ entry, after the Tejun's patch the device_del() in pci_stop_dev() removes the subordinate bus object's power directory among other things and because of that the warning triggers when we do the pci_remove_bus() in pci_remove_bus_device() for the same device. The appended patch fixes it for me, but Greg has already taken the Mika's one, so this one isn't really necessary. In case you think it would be useful to apply it anyway, please let me know and I submit it properly with a changelog etc. I can revert Mika's patch, as it would be good to catch these kinds of errors. Then we'll need to untangle the SATA/SCSI mess triggered by Bjorn in https://bugzilla.kernel.org/show_bug.cgi?id=65281. ;-) I have no objection to fixing that, the scsi sysfs handling is
Re: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Sunday, November 24, 2013 12:36:03 AM Rafael J. Wysocki wrote: On Saturday, November 23, 2013 03:07:01 PM Greg Kroah-Hartman wrote: On Sun, Nov 24, 2013 at 12:12:59AM +0100, Rafael J. Wysocki wrote: On Saturday, November 23, 2013 02:53:58 PM Greg Kroah-Hartman wrote: [...] I can revert Mika's patch, as it would be good to catch these kinds of errors. Then we'll need to untangle the SATA/SCSI mess triggered by Bjorn in https://bugzilla.kernel.org/show_bug.cgi?id=65281. ;-) I have no objection to fixing that, the scsi sysfs handling is odd to say the least... If someone can unwind it, that would be great to see happen... Well, if I'm bored to death during the xmas holidays, I may look into that. In fact, I'm not exactly sure why ata_port_detach() calls ata_tport_delete() before scsi_remove_host()? Is there any particular reason? Because that doesn't seem to be exactly right ... Rafael -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Friday, November 22, 2013 08:43:55 AM Bjorn Helgaas wrote: > [+cc Rafael, James] > > On Tue, Nov 19, 2013 at 11:18 PM, Tejun Heo wrote: > > (cc'ing Bjorn) > > > > Hello, > > > > On Tue, Nov 19, 2013 at 03:09:58PM +0200, Mika Westerberg wrote: > >> Commit bcdde7e221a8 (sysfs: make __sysfs_remove_dir() recursive) changed > >> the behavior so that directory removals will be done recursively. This > >> means that the sysfs group might already be removed if its parent directory > >> has been removed. > >> > >> The current code outputs warnings similar to following log snippet when it > >> detects that there is no group for the given kobject: > >> > >> WARNING: CPU: 0 PID: 4 at fs/sysfs/group.c:214 > >> sysfs_remove_group+0xc6/0xd0() > >> sysfs group 81c6f1e0 not found for kobject 'host7' > >> Modules linked in: > >> CPU: 0 PID: 4 Comm: kworker/0:0 Not tainted 3.12.0+ #13 > >> Hardware name: /D33217CK, BIOS > >> GKPPT10H.86A.0042.2013.0422.1439 04/22/2013 > >> Workqueue: kacpi_hotplug acpi_hotplug_work_fn > >> 0009 8801002459b0 817daab1 8801002459f8 > >> 8801002459e8 810436b8 81c6f1e0 > >> 88006d440358 88006d440188 88006e8b4c28 880100245a48 > >> Call Trace: > >> [] dump_stack+0x45/0x56 > >> [] warn_slowpath_common+0x78/0xa0 > >> [] warn_slowpath_fmt+0x47/0x50 > >> [] sysfs_remove_group+0xc6/0xd0 > >> [] dpm_sysfs_remove+0x3e/0x50 > >> [] device_del+0x40/0x1b0 > >> [] device_unregister+0xd/0x20 > >> [] scsi_remove_host+0xba/0x110 > >> [] ata_host_detach+0xc6/0x100 > >> [] ata_pci_remove_one+0x18/0x20 > >> [] pci_device_remove+0x28/0x60 > >> [] __device_release_driver+0x64/0xd0 > >> [] device_release_driver+0x1e/0x30 > >> [] bus_remove_device+0xf7/0x140 > >> [] device_del+0x121/0x1b0 > >> [] pci_stop_bus_device+0x94/0xa0 > >> [] pci_stop_bus_device+0x3b/0xa0 > >> [] pci_stop_bus_device+0x3b/0xa0 > >> [] pci_stop_and_remove_bus_device+0xd/0x20 > >> [] trim_stale_devices+0x73/0xe0 > >> [] trim_stale_devices+0xbb/0xe0 > >> [] trim_stale_devices+0xbb/0xe0 > >> [] acpiphp_check_bridge+0x7e/0xd0 > >> [] hotplug_event+0xcd/0x160 > >> [] hotplug_event_work+0x25/0x60 > >> [] acpi_hotplug_work_fn+0x17/0x22 > >> [] process_one_work+0x17a/0x430 > >> [] worker_thread+0x119/0x390 > >> [] kthread+0xcd/0xf0 > >> [] ret_from_fork+0x7c/0xb0 > > > > So, we do have cases where the parent is removed before the child. I > > suppose the parent pci bridge is removed already? AFAICS this > > shouldn't break anything but people did seem to expect the removals to > > be ordered from child to parent. Bjorn, is this something you expect > > to happened? > > I do not expect a PCI bridge to be removed before the devices below > it. We should be removing all the children before removing the parent > bridge. Precisely. > But is this related to PCI? I don't see the connection yet. I tried > to look into this a bit (my notes are at > https://bugzilla.kernel.org/show_bug.cgi?id=65281), but I haven't > figured out the big-picture problem yet. > > I don't have warm fuzzies that adding a "have we already removed this" > check is the best resolution, but maybe that's just because I don't > understand the problem. I don't think this is related to removing the parent before the child. It complains about the 'power' directory of all devices being removed it seems. Thanks! -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
Hello, On Fri, Nov 22, 2013 at 08:43:55AM -0700, Bjorn Helgaas wrote: > > So, we do have cases where the parent is removed before the child. I > > suppose the parent pci bridge is removed already? AFAICS this > > shouldn't break anything but people did seem to expect the removals to > > be ordered from child to parent. Bjorn, is this something you expect > > to happened? > > I do not expect a PCI bridge to be removed before the devices below > it. We should be removing all the children before removing the parent > bridge. > > But is this related to PCI? I don't see the connection yet. I tried I'm not sure. It was from thunderbolt and nobody is reporting it on other interconnects, so it could be. > to look into this a bit (my notes are at > https://bugzilla.kernel.org/show_bug.cgi?id=65281), but I haven't > figured out the big-picture problem yet. > > I don't have warm fuzzies that adding a "have we already removed this" > check is the best resolution, but maybe that's just because I don't > understand the problem. Yeah, the whole thing is sorta pointless. Just issuing removal and continuing on should do, IMHO. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
[+cc Rafael, James] On Tue, Nov 19, 2013 at 11:18 PM, Tejun Heo wrote: > (cc'ing Bjorn) > > Hello, > > On Tue, Nov 19, 2013 at 03:09:58PM +0200, Mika Westerberg wrote: >> Commit bcdde7e221a8 (sysfs: make __sysfs_remove_dir() recursive) changed >> the behavior so that directory removals will be done recursively. This >> means that the sysfs group might already be removed if its parent directory >> has been removed. >> >> The current code outputs warnings similar to following log snippet when it >> detects that there is no group for the given kobject: >> >> WARNING: CPU: 0 PID: 4 at fs/sysfs/group.c:214 >> sysfs_remove_group+0xc6/0xd0() >> sysfs group 81c6f1e0 not found for kobject 'host7' >> Modules linked in: >> CPU: 0 PID: 4 Comm: kworker/0:0 Not tainted 3.12.0+ #13 >> Hardware name: /D33217CK, BIOS >> GKPPT10H.86A.0042.2013.0422.1439 04/22/2013 >> Workqueue: kacpi_hotplug acpi_hotplug_work_fn >> 0009 8801002459b0 817daab1 8801002459f8 >> 8801002459e8 810436b8 81c6f1e0 >> 88006d440358 88006d440188 88006e8b4c28 880100245a48 >> Call Trace: >> [] dump_stack+0x45/0x56 >> [] warn_slowpath_common+0x78/0xa0 >> [] warn_slowpath_fmt+0x47/0x50 >> [] sysfs_remove_group+0xc6/0xd0 >> [] dpm_sysfs_remove+0x3e/0x50 >> [] device_del+0x40/0x1b0 >> [] device_unregister+0xd/0x20 >> [] scsi_remove_host+0xba/0x110 >> [] ata_host_detach+0xc6/0x100 >> [] ata_pci_remove_one+0x18/0x20 >> [] pci_device_remove+0x28/0x60 >> [] __device_release_driver+0x64/0xd0 >> [] device_release_driver+0x1e/0x30 >> [] bus_remove_device+0xf7/0x140 >> [] device_del+0x121/0x1b0 >> [] pci_stop_bus_device+0x94/0xa0 >> [] pci_stop_bus_device+0x3b/0xa0 >> [] pci_stop_bus_device+0x3b/0xa0 >> [] pci_stop_and_remove_bus_device+0xd/0x20 >> [] trim_stale_devices+0x73/0xe0 >> [] trim_stale_devices+0xbb/0xe0 >> [] trim_stale_devices+0xbb/0xe0 >> [] acpiphp_check_bridge+0x7e/0xd0 >> [] hotplug_event+0xcd/0x160 >> [] hotplug_event_work+0x25/0x60 >> [] acpi_hotplug_work_fn+0x17/0x22 >> [] process_one_work+0x17a/0x430 >> [] worker_thread+0x119/0x390 >> [] kthread+0xcd/0xf0 >> [] ret_from_fork+0x7c/0xb0 > > So, we do have cases where the parent is removed before the child. I > suppose the parent pci bridge is removed already? AFAICS this > shouldn't break anything but people did seem to expect the removals to > be ordered from child to parent. Bjorn, is this something you expect > to happened? I do not expect a PCI bridge to be removed before the devices below it. We should be removing all the children before removing the parent bridge. But is this related to PCI? I don't see the connection yet. I tried to look into this a bit (my notes are at https://bugzilla.kernel.org/show_bug.cgi?id=65281), but I haven't figured out the big-picture problem yet. I don't have warm fuzzies that adding a "have we already removed this" check is the best resolution, but maybe that's just because I don't understand the problem. Bjorn >> I'm not 100% sure that this is the correct solution. It seem to fix my case >> but I might be missing something as I'm not that familiar with sysfs. > > Yeah, looks okay to me for now. One nit at the end tho. > > I find requiring removal of each sysfs attribute when the whole node > is going away rather weird. It forced us to have extra code which > does whole bunch of hash table lookups and deletion operations and the > only thing that achieved was either triggering warning if somebody did > it in the wrong order or spuriously, or leaking memory if somebody > forgot some without any way to find out about them. > > Now, all those are harmlessly unnecessary and we're adding more logic > to suppress warnings on specific cases. In the longer term, we > probably just wanna drop all the unnecessary removal logics and > warnings. > >> + /* >> + * Sysfs directories are now removed recursively by >> + * sysfs_remove_dir(). This means that this function can be called >> + * multiple times on the same group. If the parent directory is >> + * already removed we don't do anything here. >> + */ > > The function won't be called multiple times but may be called on a > group whose kobj whose sysfs entry is already removed in which case > all its groups are guaranteed to be already removed. > > Can you please update the comment to reflect the above? > > With that, > > Acked-by: Tejun Heo > > Thanks. > > -- > tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
[+cc Rafael, James] On Tue, Nov 19, 2013 at 11:18 PM, Tejun Heo t...@kernel.org wrote: (cc'ing Bjorn) Hello, On Tue, Nov 19, 2013 at 03:09:58PM +0200, Mika Westerberg wrote: Commit bcdde7e221a8 (sysfs: make __sysfs_remove_dir() recursive) changed the behavior so that directory removals will be done recursively. This means that the sysfs group might already be removed if its parent directory has been removed. The current code outputs warnings similar to following log snippet when it detects that there is no group for the given kobject: WARNING: CPU: 0 PID: 4 at fs/sysfs/group.c:214 sysfs_remove_group+0xc6/0xd0() sysfs group 81c6f1e0 not found for kobject 'host7' Modules linked in: CPU: 0 PID: 4 Comm: kworker/0:0 Not tainted 3.12.0+ #13 Hardware name: /D33217CK, BIOS GKPPT10H.86A.0042.2013.0422.1439 04/22/2013 Workqueue: kacpi_hotplug acpi_hotplug_work_fn 0009 8801002459b0 817daab1 8801002459f8 8801002459e8 810436b8 81c6f1e0 88006d440358 88006d440188 88006e8b4c28 880100245a48 Call Trace: [817daab1] dump_stack+0x45/0x56 [810436b8] warn_slowpath_common+0x78/0xa0 [81043727] warn_slowpath_fmt+0x47/0x50 [811ae526] sysfs_remove_group+0xc6/0xd0 [81432f7e] dpm_sysfs_remove+0x3e/0x50 [8142a0d0] device_del+0x40/0x1b0 [8142a24d] device_unregister+0xd/0x20 [8144131a] scsi_remove_host+0xba/0x110 [8145f526] ata_host_detach+0xc6/0x100 [8145f578] ata_pci_remove_one+0x18/0x20 [812e8f48] pci_device_remove+0x28/0x60 [8142d854] __device_release_driver+0x64/0xd0 [8142d8de] device_release_driver+0x1e/0x30 [8142d257] bus_remove_device+0xf7/0x140 [8142a1b1] device_del+0x121/0x1b0 [812e43d4] pci_stop_bus_device+0x94/0xa0 [812e437b] pci_stop_bus_device+0x3b/0xa0 [812e437b] pci_stop_bus_device+0x3b/0xa0 [812e44dd] pci_stop_and_remove_bus_device+0xd/0x20 [812fc743] trim_stale_devices+0x73/0xe0 [812fc78b] trim_stale_devices+0xbb/0xe0 [812fc78b] trim_stale_devices+0xbb/0xe0 [812fcb6e] acpiphp_check_bridge+0x7e/0xd0 [812fd90d] hotplug_event+0xcd/0x160 [812fd9c5] hotplug_event_work+0x25/0x60 [81316749] acpi_hotplug_work_fn+0x17/0x22 [8105cf3a] process_one_work+0x17a/0x430 [8105db29] worker_thread+0x119/0x390 [81063a5d] kthread+0xcd/0xf0 [817eb33c] ret_from_fork+0x7c/0xb0 So, we do have cases where the parent is removed before the child. I suppose the parent pci bridge is removed already? AFAICS this shouldn't break anything but people did seem to expect the removals to be ordered from child to parent. Bjorn, is this something you expect to happened? I do not expect a PCI bridge to be removed before the devices below it. We should be removing all the children before removing the parent bridge. But is this related to PCI? I don't see the connection yet. I tried to look into this a bit (my notes are at https://bugzilla.kernel.org/show_bug.cgi?id=65281), but I haven't figured out the big-picture problem yet. I don't have warm fuzzies that adding a have we already removed this check is the best resolution, but maybe that's just because I don't understand the problem. Bjorn I'm not 100% sure that this is the correct solution. It seem to fix my case but I might be missing something as I'm not that familiar with sysfs. Yeah, looks okay to me for now. One nit at the end tho. I find requiring removal of each sysfs attribute when the whole node is going away rather weird. It forced us to have extra code which does whole bunch of hash table lookups and deletion operations and the only thing that achieved was either triggering warning if somebody did it in the wrong order or spuriously, or leaking memory if somebody forgot some without any way to find out about them. Now, all those are harmlessly unnecessary and we're adding more logic to suppress warnings on specific cases. In the longer term, we probably just wanna drop all the unnecessary removal logics and warnings. + /* + * Sysfs directories are now removed recursively by + * sysfs_remove_dir(). This means that this function can be called + * multiple times on the same group. If the parent directory is + * already removed we don't do anything here. + */ The function won't be called multiple times but may be called on a group whose kobj whose sysfs entry is already removed in which case all its groups are guaranteed to be already removed. Can you please update the comment to reflect the above? With that, Acked-by: Tejun Heo t...@kernel.org Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of
Re: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
Hello, On Fri, Nov 22, 2013 at 08:43:55AM -0700, Bjorn Helgaas wrote: So, we do have cases where the parent is removed before the child. I suppose the parent pci bridge is removed already? AFAICS this shouldn't break anything but people did seem to expect the removals to be ordered from child to parent. Bjorn, is this something you expect to happened? I do not expect a PCI bridge to be removed before the devices below it. We should be removing all the children before removing the parent bridge. But is this related to PCI? I don't see the connection yet. I tried I'm not sure. It was from thunderbolt and nobody is reporting it on other interconnects, so it could be. to look into this a bit (my notes are at https://bugzilla.kernel.org/show_bug.cgi?id=65281), but I haven't figured out the big-picture problem yet. I don't have warm fuzzies that adding a have we already removed this check is the best resolution, but maybe that's just because I don't understand the problem. Yeah, the whole thing is sorta pointless. Just issuing removal and continuing on should do, IMHO. Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
On Friday, November 22, 2013 08:43:55 AM Bjorn Helgaas wrote: [+cc Rafael, James] On Tue, Nov 19, 2013 at 11:18 PM, Tejun Heo t...@kernel.org wrote: (cc'ing Bjorn) Hello, On Tue, Nov 19, 2013 at 03:09:58PM +0200, Mika Westerberg wrote: Commit bcdde7e221a8 (sysfs: make __sysfs_remove_dir() recursive) changed the behavior so that directory removals will be done recursively. This means that the sysfs group might already be removed if its parent directory has been removed. The current code outputs warnings similar to following log snippet when it detects that there is no group for the given kobject: WARNING: CPU: 0 PID: 4 at fs/sysfs/group.c:214 sysfs_remove_group+0xc6/0xd0() sysfs group 81c6f1e0 not found for kobject 'host7' Modules linked in: CPU: 0 PID: 4 Comm: kworker/0:0 Not tainted 3.12.0+ #13 Hardware name: /D33217CK, BIOS GKPPT10H.86A.0042.2013.0422.1439 04/22/2013 Workqueue: kacpi_hotplug acpi_hotplug_work_fn 0009 8801002459b0 817daab1 8801002459f8 8801002459e8 810436b8 81c6f1e0 88006d440358 88006d440188 88006e8b4c28 880100245a48 Call Trace: [817daab1] dump_stack+0x45/0x56 [810436b8] warn_slowpath_common+0x78/0xa0 [81043727] warn_slowpath_fmt+0x47/0x50 [811ae526] sysfs_remove_group+0xc6/0xd0 [81432f7e] dpm_sysfs_remove+0x3e/0x50 [8142a0d0] device_del+0x40/0x1b0 [8142a24d] device_unregister+0xd/0x20 [8144131a] scsi_remove_host+0xba/0x110 [8145f526] ata_host_detach+0xc6/0x100 [8145f578] ata_pci_remove_one+0x18/0x20 [812e8f48] pci_device_remove+0x28/0x60 [8142d854] __device_release_driver+0x64/0xd0 [8142d8de] device_release_driver+0x1e/0x30 [8142d257] bus_remove_device+0xf7/0x140 [8142a1b1] device_del+0x121/0x1b0 [812e43d4] pci_stop_bus_device+0x94/0xa0 [812e437b] pci_stop_bus_device+0x3b/0xa0 [812e437b] pci_stop_bus_device+0x3b/0xa0 [812e44dd] pci_stop_and_remove_bus_device+0xd/0x20 [812fc743] trim_stale_devices+0x73/0xe0 [812fc78b] trim_stale_devices+0xbb/0xe0 [812fc78b] trim_stale_devices+0xbb/0xe0 [812fcb6e] acpiphp_check_bridge+0x7e/0xd0 [812fd90d] hotplug_event+0xcd/0x160 [812fd9c5] hotplug_event_work+0x25/0x60 [81316749] acpi_hotplug_work_fn+0x17/0x22 [8105cf3a] process_one_work+0x17a/0x430 [8105db29] worker_thread+0x119/0x390 [81063a5d] kthread+0xcd/0xf0 [817eb33c] ret_from_fork+0x7c/0xb0 So, we do have cases where the parent is removed before the child. I suppose the parent pci bridge is removed already? AFAICS this shouldn't break anything but people did seem to expect the removals to be ordered from child to parent. Bjorn, is this something you expect to happened? I do not expect a PCI bridge to be removed before the devices below it. We should be removing all the children before removing the parent bridge. Precisely. But is this related to PCI? I don't see the connection yet. I tried to look into this a bit (my notes are at https://bugzilla.kernel.org/show_bug.cgi?id=65281), but I haven't figured out the big-picture problem yet. I don't have warm fuzzies that adding a have we already removed this check is the best resolution, but maybe that's just because I don't understand the problem. I don't think this is related to removing the parent before the child. It complains about the 'power' directory of all devices being removed it seems. Thanks! -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
(cc'ing Bjorn) Hello, On Tue, Nov 19, 2013 at 03:09:58PM +0200, Mika Westerberg wrote: > Commit bcdde7e221a8 (sysfs: make __sysfs_remove_dir() recursive) changed > the behavior so that directory removals will be done recursively. This > means that the sysfs group might already be removed if its parent directory > has been removed. > > The current code outputs warnings similar to following log snippet when it > detects that there is no group for the given kobject: > > WARNING: CPU: 0 PID: 4 at fs/sysfs/group.c:214 sysfs_remove_group+0xc6/0xd0() > sysfs group 81c6f1e0 not found for kobject 'host7' > Modules linked in: > CPU: 0 PID: 4 Comm: kworker/0:0 Not tainted 3.12.0+ #13 > Hardware name: /D33217CK, BIOS > GKPPT10H.86A.0042.2013.0422.1439 04/22/2013 > Workqueue: kacpi_hotplug acpi_hotplug_work_fn > 0009 8801002459b0 817daab1 8801002459f8 > 8801002459e8 810436b8 81c6f1e0 > 88006d440358 88006d440188 88006e8b4c28 880100245a48 > Call Trace: > [] dump_stack+0x45/0x56 > [] warn_slowpath_common+0x78/0xa0 > [] warn_slowpath_fmt+0x47/0x50 > [] sysfs_remove_group+0xc6/0xd0 > [] dpm_sysfs_remove+0x3e/0x50 > [] device_del+0x40/0x1b0 > [] device_unregister+0xd/0x20 > [] scsi_remove_host+0xba/0x110 > [] ata_host_detach+0xc6/0x100 > [] ata_pci_remove_one+0x18/0x20 > [] pci_device_remove+0x28/0x60 > [] __device_release_driver+0x64/0xd0 > [] device_release_driver+0x1e/0x30 > [] bus_remove_device+0xf7/0x140 > [] device_del+0x121/0x1b0 > [] pci_stop_bus_device+0x94/0xa0 > [] pci_stop_bus_device+0x3b/0xa0 > [] pci_stop_bus_device+0x3b/0xa0 > [] pci_stop_and_remove_bus_device+0xd/0x20 > [] trim_stale_devices+0x73/0xe0 > [] trim_stale_devices+0xbb/0xe0 > [] trim_stale_devices+0xbb/0xe0 > [] acpiphp_check_bridge+0x7e/0xd0 > [] hotplug_event+0xcd/0x160 > [] hotplug_event_work+0x25/0x60 > [] acpi_hotplug_work_fn+0x17/0x22 > [] process_one_work+0x17a/0x430 > [] worker_thread+0x119/0x390 > [] kthread+0xcd/0xf0 > [] ret_from_fork+0x7c/0xb0 So, we do have cases where the parent is removed before the child. I suppose the parent pci bridge is removed already? AFAICS this shouldn't break anything but people did seem to expect the removals to be ordered from child to parent. Bjorn, is this something you expect to happened? > I'm not 100% sure that this is the correct solution. It seem to fix my case > but I might be missing something as I'm not that familiar with sysfs. Yeah, looks okay to me for now. One nit at the end tho. I find requiring removal of each sysfs attribute when the whole node is going away rather weird. It forced us to have extra code which does whole bunch of hash table lookups and deletion operations and the only thing that achieved was either triggering warning if somebody did it in the wrong order or spuriously, or leaking memory if somebody forgot some without any way to find out about them. Now, all those are harmlessly unnecessary and we're adding more logic to suppress warnings on specific cases. In the longer term, we probably just wanna drop all the unnecessary removal logics and warnings. > + /* > + * Sysfs directories are now removed recursively by > + * sysfs_remove_dir(). This means that this function can be called > + * multiple times on the same group. If the parent directory is > + * already removed we don't do anything here. > + */ The function won't be called multiple times but may be called on a group whose kobj whose sysfs entry is already removed in which case all its groups are guaranteed to be already removed. Can you please update the comment to reflect the above? With that, Acked-by: Tejun Heo Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
Commit bcdde7e221a8 (sysfs: make __sysfs_remove_dir() recursive) changed
the behavior so that directory removals will be done recursively. This
means that the sysfs group might already be removed if its parent directory
has been removed.
The current code outputs warnings similar to following log snippet when it
detects that there is no group for the given kobject:
WARNING: CPU: 0 PID: 4 at fs/sysfs/group.c:214 sysfs_remove_group+0xc6/0xd0()
sysfs group 81c6f1e0 not found for kobject 'host7'
Modules linked in:
CPU: 0 PID: 4 Comm: kworker/0:0 Not tainted 3.12.0+ #13
Hardware name: /D33217CK, BIOS
GKPPT10H.86A.0042.2013.0422.1439 04/22/2013
Workqueue: kacpi_hotplug acpi_hotplug_work_fn
0009 8801002459b0 817daab1 8801002459f8
8801002459e8 810436b8 81c6f1e0
88006d440358 88006d440188 88006e8b4c28 880100245a48
Call Trace:
[] dump_stack+0x45/0x56
[] warn_slowpath_common+0x78/0xa0
[] warn_slowpath_fmt+0x47/0x50
[] ? sysfs_get_dirent_ns+0x49/0x70
[] sysfs_remove_group+0xc6/0xd0
[] dpm_sysfs_remove+0x3e/0x50
[] device_del+0x40/0x1b0
[] device_unregister+0xd/0x20
[] scsi_remove_host+0xba/0x110
[] ata_host_detach+0xc6/0x100
[] ata_pci_remove_one+0x18/0x20
[] pci_device_remove+0x28/0x60
[] __device_release_driver+0x64/0xd0
[] device_release_driver+0x1e/0x30
[] bus_remove_device+0xf7/0x140
[] device_del+0x121/0x1b0
[] pci_stop_bus_device+0x94/0xa0
[] pci_stop_bus_device+0x3b/0xa0
[] pci_stop_bus_device+0x3b/0xa0
[] pci_stop_and_remove_bus_device+0xd/0x20
[] trim_stale_devices+0x73/0xe0
[] trim_stale_devices+0xbb/0xe0
[] trim_stale_devices+0xbb/0xe0
[] acpiphp_check_bridge+0x7e/0xd0
[] hotplug_event+0xcd/0x160
[] hotplug_event_work+0x25/0x60
[] acpi_hotplug_work_fn+0x17/0x22
[] process_one_work+0x17a/0x430
[] worker_thread+0x119/0x390
[] ? manage_workers.isra.25+0x2a0/0x2a0
[] kthread+0xcd/0xf0
[] ? kthread_create_on_node+0x180/0x180
[] ret_from_fork+0x7c/0xb0
[] ? kthread_create_on_node+0x180/0x180
On this particular machine I see ~16 of these message during Thunderbolt
hot-unplug.
Fix this in similar way that was done for sysfs_remove_one() by checking
if the parent directory has already been removed and bailing out early.
Signed-off-by: Mika Westerberg
---
I'm not 100% sure that this is the correct solution. It seem to fix my case
but I might be missing something as I'm not that familiar with sysfs.
fs/sysfs/group.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 1898a10e38ce..8951bfb4e567 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -206,6 +206,15 @@ void sysfs_remove_group(struct kobject *kobj,
struct sysfs_dirent *dir_sd = kobj->sd;
struct sysfs_dirent *sd;
+ /*
+* Sysfs directories are now removed recursively by
+* sysfs_remove_dir(). This means that this function can be called
+* multiple times on the same group. If the parent directory is
+* already removed we don't do anything here.
+*/
+ if (dir_sd->s_flags & SYSFS_FLAG_REMOVED)
+ return;
+
if (grp->name) {
sd = sysfs_get_dirent(dir_sd, grp->name);
if (!sd) {
--
1.8.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
Commit bcdde7e221a8 (sysfs: make __sysfs_remove_dir() recursive) changed
the behavior so that directory removals will be done recursively. This
means that the sysfs group might already be removed if its parent directory
has been removed.
The current code outputs warnings similar to following log snippet when it
detects that there is no group for the given kobject:
WARNING: CPU: 0 PID: 4 at fs/sysfs/group.c:214 sysfs_remove_group+0xc6/0xd0()
sysfs group 81c6f1e0 not found for kobject 'host7'
Modules linked in:
CPU: 0 PID: 4 Comm: kworker/0:0 Not tainted 3.12.0+ #13
Hardware name: /D33217CK, BIOS
GKPPT10H.86A.0042.2013.0422.1439 04/22/2013
Workqueue: kacpi_hotplug acpi_hotplug_work_fn
0009 8801002459b0 817daab1 8801002459f8
8801002459e8 810436b8 81c6f1e0
88006d440358 88006d440188 88006e8b4c28 880100245a48
Call Trace:
[817daab1] dump_stack+0x45/0x56
[810436b8] warn_slowpath_common+0x78/0xa0
[81043727] warn_slowpath_fmt+0x47/0x50
[811ad319] ? sysfs_get_dirent_ns+0x49/0x70
[811ae526] sysfs_remove_group+0xc6/0xd0
[81432f7e] dpm_sysfs_remove+0x3e/0x50
[8142a0d0] device_del+0x40/0x1b0
[8142a24d] device_unregister+0xd/0x20
[8144131a] scsi_remove_host+0xba/0x110
[8145f526] ata_host_detach+0xc6/0x100
[8145f578] ata_pci_remove_one+0x18/0x20
[812e8f48] pci_device_remove+0x28/0x60
[8142d854] __device_release_driver+0x64/0xd0
[8142d8de] device_release_driver+0x1e/0x30
[8142d257] bus_remove_device+0xf7/0x140
[8142a1b1] device_del+0x121/0x1b0
[812e43d4] pci_stop_bus_device+0x94/0xa0
[812e437b] pci_stop_bus_device+0x3b/0xa0
[812e437b] pci_stop_bus_device+0x3b/0xa0
[812e44dd] pci_stop_and_remove_bus_device+0xd/0x20
[812fc743] trim_stale_devices+0x73/0xe0
[812fc78b] trim_stale_devices+0xbb/0xe0
[812fc78b] trim_stale_devices+0xbb/0xe0
[812fcb6e] acpiphp_check_bridge+0x7e/0xd0
[812fd90d] hotplug_event+0xcd/0x160
[812fd9c5] hotplug_event_work+0x25/0x60
[81316749] acpi_hotplug_work_fn+0x17/0x22
[8105cf3a] process_one_work+0x17a/0x430
[8105db29] worker_thread+0x119/0x390
[8105da10] ? manage_workers.isra.25+0x2a0/0x2a0
[81063a5d] kthread+0xcd/0xf0
[81063990] ? kthread_create_on_node+0x180/0x180
[817eb33c] ret_from_fork+0x7c/0xb0
[81063990] ? kthread_create_on_node+0x180/0x180
On this particular machine I see ~16 of these message during Thunderbolt
hot-unplug.
Fix this in similar way that was done for sysfs_remove_one() by checking
if the parent directory has already been removed and bailing out early.
Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com
---
I'm not 100% sure that this is the correct solution. It seem to fix my case
but I might be missing something as I'm not that familiar with sysfs.
fs/sysfs/group.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 1898a10e38ce..8951bfb4e567 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -206,6 +206,15 @@ void sysfs_remove_group(struct kobject *kobj,
struct sysfs_dirent *dir_sd = kobj-sd;
struct sysfs_dirent *sd;
+ /*
+* Sysfs directories are now removed recursively by
+* sysfs_remove_dir(). This means that this function can be called
+* multiple times on the same group. If the parent directory is
+* already removed we don't do anything here.
+*/
+ if (dir_sd-s_flags SYSFS_FLAG_REMOVED)
+ return;
+
if (grp-name) {
sd = sysfs_get_dirent(dir_sd, grp-name);
if (!sd) {
--
1.8.4.3
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group()
(cc'ing Bjorn) Hello, On Tue, Nov 19, 2013 at 03:09:58PM +0200, Mika Westerberg wrote: Commit bcdde7e221a8 (sysfs: make __sysfs_remove_dir() recursive) changed the behavior so that directory removals will be done recursively. This means that the sysfs group might already be removed if its parent directory has been removed. The current code outputs warnings similar to following log snippet when it detects that there is no group for the given kobject: WARNING: CPU: 0 PID: 4 at fs/sysfs/group.c:214 sysfs_remove_group+0xc6/0xd0() sysfs group 81c6f1e0 not found for kobject 'host7' Modules linked in: CPU: 0 PID: 4 Comm: kworker/0:0 Not tainted 3.12.0+ #13 Hardware name: /D33217CK, BIOS GKPPT10H.86A.0042.2013.0422.1439 04/22/2013 Workqueue: kacpi_hotplug acpi_hotplug_work_fn 0009 8801002459b0 817daab1 8801002459f8 8801002459e8 810436b8 81c6f1e0 88006d440358 88006d440188 88006e8b4c28 880100245a48 Call Trace: [817daab1] dump_stack+0x45/0x56 [810436b8] warn_slowpath_common+0x78/0xa0 [81043727] warn_slowpath_fmt+0x47/0x50 [811ae526] sysfs_remove_group+0xc6/0xd0 [81432f7e] dpm_sysfs_remove+0x3e/0x50 [8142a0d0] device_del+0x40/0x1b0 [8142a24d] device_unregister+0xd/0x20 [8144131a] scsi_remove_host+0xba/0x110 [8145f526] ata_host_detach+0xc6/0x100 [8145f578] ata_pci_remove_one+0x18/0x20 [812e8f48] pci_device_remove+0x28/0x60 [8142d854] __device_release_driver+0x64/0xd0 [8142d8de] device_release_driver+0x1e/0x30 [8142d257] bus_remove_device+0xf7/0x140 [8142a1b1] device_del+0x121/0x1b0 [812e43d4] pci_stop_bus_device+0x94/0xa0 [812e437b] pci_stop_bus_device+0x3b/0xa0 [812e437b] pci_stop_bus_device+0x3b/0xa0 [812e44dd] pci_stop_and_remove_bus_device+0xd/0x20 [812fc743] trim_stale_devices+0x73/0xe0 [812fc78b] trim_stale_devices+0xbb/0xe0 [812fc78b] trim_stale_devices+0xbb/0xe0 [812fcb6e] acpiphp_check_bridge+0x7e/0xd0 [812fd90d] hotplug_event+0xcd/0x160 [812fd9c5] hotplug_event_work+0x25/0x60 [81316749] acpi_hotplug_work_fn+0x17/0x22 [8105cf3a] process_one_work+0x17a/0x430 [8105db29] worker_thread+0x119/0x390 [81063a5d] kthread+0xcd/0xf0 [817eb33c] ret_from_fork+0x7c/0xb0 So, we do have cases where the parent is removed before the child. I suppose the parent pci bridge is removed already? AFAICS this shouldn't break anything but people did seem to expect the removals to be ordered from child to parent. Bjorn, is this something you expect to happened? I'm not 100% sure that this is the correct solution. It seem to fix my case but I might be missing something as I'm not that familiar with sysfs. Yeah, looks okay to me for now. One nit at the end tho. I find requiring removal of each sysfs attribute when the whole node is going away rather weird. It forced us to have extra code which does whole bunch of hash table lookups and deletion operations and the only thing that achieved was either triggering warning if somebody did it in the wrong order or spuriously, or leaking memory if somebody forgot some without any way to find out about them. Now, all those are harmlessly unnecessary and we're adding more logic to suppress warnings on specific cases. In the longer term, we probably just wanna drop all the unnecessary removal logics and warnings. + /* + * Sysfs directories are now removed recursively by + * sysfs_remove_dir(). This means that this function can be called + * multiple times on the same group. If the parent directory is + * already removed we don't do anything here. + */ The function won't be called multiple times but may be called on a group whose kobj whose sysfs entry is already removed in which case all its groups are guaranteed to be already removed. Can you please update the comment to reflect the above? With that, Acked-by: Tejun Heo t...@kernel.org Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
