| Message ID | 1372177330-28013-2-git-send-email-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> > > With Thunderbolt you can chain devices: connect a new devices to plugged > one. In this case the slot is already enabled, but we still want to look > for new devices behind it. > > We're going to reuse enable_device() for rescan for new devices on the > enabled slot. Let's push the check up by stack. > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > drivers/pci/hotplug/acpiphp_glue.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c > index 59df857..b983e29 100644 > --- a/drivers/pci/hotplug/acpiphp_glue.c > +++ b/drivers/pci/hotplug/acpiphp_glue.c > @@ -688,9 +688,6 @@ static int __ref enable_device(struct acpiphp_slot *slot) > int num, max, pass; > LIST_HEAD(add_list); > > - if (slot->flags & SLOT_ENABLED) > - goto err_exit; > - > list_for_each_entry(func, &slot->funcs, sibling) > acpiphp_bus_add(func); > > @@ -1242,6 +1239,8 @@ int acpiphp_enable_slot(struct acpiphp_slot *slot) > goto err_exit; > > if (get_slot_status(slot) == ACPI_STA_ALL) { > + if (slot->flags & SLOT_ENABLED) > + goto err_exit; Why do we check for SLOT_ENABLED at all? I think we're handling a Bus Check notification, which means "re-enumerate on the device tree starting from the notification point." It doesn't say anything about skipping the re-enumeration if we find a device that's already enabled. It seems like we ought to just re-enumerate all the way down in case a device was added farther down in the tree (which is what it sounds like Thunderbolt is doing). > /* configure all functions */ > retval = enable_device(slot); > if (retval) > -- > 1.8.3.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas wrote: > On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> > > > > With Thunderbolt you can chain devices: connect a new devices to plugged > > one. In this case the slot is already enabled, but we still want to look > > for new devices behind it. > > > > We're going to reuse enable_device() for rescan for new devices on the > > enabled slot. Let's push the check up by stack. > > > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > --- > > drivers/pci/hotplug/acpiphp_glue.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c > > index 59df857..b983e29 100644 > > --- a/drivers/pci/hotplug/acpiphp_glue.c > > +++ b/drivers/pci/hotplug/acpiphp_glue.c > > @@ -688,9 +688,6 @@ static int __ref enable_device(struct acpiphp_slot *slot) > > int num, max, pass; > > LIST_HEAD(add_list); > > > > - if (slot->flags & SLOT_ENABLED) > > - goto err_exit; > > - > > list_for_each_entry(func, &slot->funcs, sibling) > > acpiphp_bus_add(func); > > > > @@ -1242,6 +1239,8 @@ int acpiphp_enable_slot(struct acpiphp_slot *slot) > > goto err_exit; > > > > if (get_slot_status(slot) == ACPI_STA_ALL) { > > + if (slot->flags & SLOT_ENABLED) > > + goto err_exit; > > Why do we check for SLOT_ENABLED at all? I think we're handling a Bus > Check notification, which means "re-enumerate on the device tree > starting from the notification point." It doesn't say anything about > skipping the re-enumeration if we find a device that's already > enabled. > > It seems like we ought to just re-enumerate all the way down in case a > device was added farther down in the tree (which is what it sounds > like Thunderbolt is doing). Okay, we will do more cleanup here. The tricky part here is that acpiphp_enable_slot() is exposed to userspace directly: /sys/bus/pci/slots/*/power files. I wounder if complete drop of the check is an ABI change.
Bjorn Helgaas wrote: > On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> > > > > With Thunderbolt you can chain devices: connect a new devices to plugged > > one. In this case the slot is already enabled, but we still want to look > > for new devices behind it. > > > > We're going to reuse enable_device() for rescan for new devices on the > > enabled slot. Let's push the check up by stack. > > > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > --- > > drivers/pci/hotplug/acpiphp_glue.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c > > index 59df857..b983e29 100644 > > --- a/drivers/pci/hotplug/acpiphp_glue.c > > +++ b/drivers/pci/hotplug/acpiphp_glue.c > > @@ -688,9 +688,6 @@ static int __ref enable_device(struct acpiphp_slot *slot) > > int num, max, pass; > > LIST_HEAD(add_list); > > > > - if (slot->flags & SLOT_ENABLED) > > - goto err_exit; > > - > > list_for_each_entry(func, &slot->funcs, sibling) > > acpiphp_bus_add(func); > > > > @@ -1242,6 +1239,8 @@ int acpiphp_enable_slot(struct acpiphp_slot *slot) > > goto err_exit; > > > > if (get_slot_status(slot) == ACPI_STA_ALL) { > > + if (slot->flags & SLOT_ENABLED) > > + goto err_exit; > > Why do we check for SLOT_ENABLED at all? I think we're handling a Bus > Check notification, which means "re-enumerate on the device tree > starting from the notification point." It doesn't say anything about > skipping the re-enumeration if we find a device that's already > enabled. > > It seems like we ought to just re-enumerate all the way down in case a > device was added farther down in the tree (which is what it sounds > like Thunderbolt is doing). Currently (with patchset applied), we have two users of acpiphp_enable_slot(): - /sys/bus/pci/slots/*/power - ACPI_NOTIFY_BUS_CHECK in _handle_hotplug_event_func(). Both are not related to Thunderbolt. Although, I think remove the check is good idea, I prefer to keep it separate from Thunderbolt enabling patchset, since it will change sysfs ABI a bit and can potentially affect othe ACPI PCI hotplug implementations.
On Fri, Jun 28, 2013 at 3:51 AM, Kirill A. Shutemov <kirill.shutemov@linux.intel.com> wrote: > Bjorn Helgaas wrote: >> On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg >> <mika.westerberg@linux.intel.com> wrote: >> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> >> > >> > With Thunderbolt you can chain devices: connect a new devices to plugged >> > one. In this case the slot is already enabled, but we still want to look >> > for new devices behind it. >> > >> > We're going to reuse enable_device() for rescan for new devices on the >> > enabled slot. Let's push the check up by stack. >> > >> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> >> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> >> > --- >> > drivers/pci/hotplug/acpiphp_glue.c | 5 ++--- >> > 1 file changed, 2 insertions(+), 3 deletions(-) >> > >> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c >> > index 59df857..b983e29 100644 >> > --- a/drivers/pci/hotplug/acpiphp_glue.c >> > +++ b/drivers/pci/hotplug/acpiphp_glue.c >> > @@ -688,9 +688,6 @@ static int __ref enable_device(struct acpiphp_slot *slot) >> > int num, max, pass; >> > LIST_HEAD(add_list); >> > >> > - if (slot->flags & SLOT_ENABLED) >> > - goto err_exit; >> > - >> > list_for_each_entry(func, &slot->funcs, sibling) >> > acpiphp_bus_add(func); >> > >> > @@ -1242,6 +1239,8 @@ int acpiphp_enable_slot(struct acpiphp_slot *slot) >> > goto err_exit; >> > >> > if (get_slot_status(slot) == ACPI_STA_ALL) { >> > + if (slot->flags & SLOT_ENABLED) >> > + goto err_exit; >> >> Why do we check for SLOT_ENABLED at all? I think we're handling a Bus >> Check notification, which means "re-enumerate on the device tree >> starting from the notification point." It doesn't say anything about >> skipping the re-enumeration if we find a device that's already >> enabled. >> >> It seems like we ought to just re-enumerate all the way down in case a >> device was added farther down in the tree (which is what it sounds >> like Thunderbolt is doing). > > Currently (with patchset applied), we have two users of > acpiphp_enable_slot(): > > - /sys/bus/pci/slots/*/power > - ACPI_NOTIFY_BUS_CHECK in _handle_hotplug_event_func(). > > Both are not related to Thunderbolt. > > Although, I think remove the check is good idea, I prefer to keep it > separate from Thunderbolt enabling patchset, since it will change sysfs > ABI a bit and can potentially affect othe ACPI PCI hotplug > implementations. I'll think about this some more, but if we can make a change that simplifies things and makes them more spec-compliant, and also happens to make Thunderbolt work, that sounds better than fixing Thunderbolt while leaving the wart there. If we only fix Thunderbolt, it just feels like we're adding to an ever-growing "deferred maintenance" list. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, June 28, 2013 11:00:31 AM Bjorn Helgaas wrote: > On Fri, Jun 28, 2013 at 3:51 AM, Kirill A. Shutemov > <kirill.shutemov@linux.intel.com> wrote: > > Bjorn Helgaas wrote: > >> On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg > >> <mika.westerberg@linux.intel.com> wrote: > >> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> > >> > > >> > With Thunderbolt you can chain devices: connect a new devices to plugged > >> > one. In this case the slot is already enabled, but we still want to look > >> > for new devices behind it. > >> > > >> > We're going to reuse enable_device() for rescan for new devices on the > >> > enabled slot. Let's push the check up by stack. > >> > > >> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > >> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > >> > --- > >> > drivers/pci/hotplug/acpiphp_glue.c | 5 ++--- > >> > 1 file changed, 2 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c > >> > index 59df857..b983e29 100644 > >> > --- a/drivers/pci/hotplug/acpiphp_glue.c > >> > +++ b/drivers/pci/hotplug/acpiphp_glue.c > >> > @@ -688,9 +688,6 @@ static int __ref enable_device(struct acpiphp_slot *slot) > >> > int num, max, pass; > >> > LIST_HEAD(add_list); > >> > > >> > - if (slot->flags & SLOT_ENABLED) > >> > - goto err_exit; > >> > - > >> > list_for_each_entry(func, &slot->funcs, sibling) > >> > acpiphp_bus_add(func); > >> > > >> > @@ -1242,6 +1239,8 @@ int acpiphp_enable_slot(struct acpiphp_slot *slot) > >> > goto err_exit; > >> > > >> > if (get_slot_status(slot) == ACPI_STA_ALL) { > >> > + if (slot->flags & SLOT_ENABLED) > >> > + goto err_exit; > >> > >> Why do we check for SLOT_ENABLED at all? I think we're handling a Bus > >> Check notification, which means "re-enumerate on the device tree > >> starting from the notification point." It doesn't say anything about > >> skipping the re-enumeration if we find a device that's already > >> enabled. > >> > >> It seems like we ought to just re-enumerate all the way down in case a > >> device was added farther down in the tree (which is what it sounds > >> like Thunderbolt is doing). > > > > Currently (with patchset applied), we have two users of > > acpiphp_enable_slot(): > > > > - /sys/bus/pci/slots/*/power > > - ACPI_NOTIFY_BUS_CHECK in _handle_hotplug_event_func(). > > > > Both are not related to Thunderbolt. > > > > Although, I think remove the check is good idea, I prefer to keep it > > separate from Thunderbolt enabling patchset, since it will change sysfs > > ABI a bit and can potentially affect othe ACPI PCI hotplug > > implementations. > > I'll think about this some more, but if we can make a change that > simplifies things and makes them more spec-compliant, and also happens > to make Thunderbolt work, that sounds better than fixing Thunderbolt > while leaving the wart there. > > If we only fix Thunderbolt, it just feels like we're adding to an > ever-growing "deferred maintenance" list. I agree. That change may be done in a separate patch, but it should be included in the series. Thanks, Rafael
On Fri, Jun 28, 2013 at 08:54:45PM +0200, Rafael J. Wysocki wrote: > On Friday, June 28, 2013 11:00:31 AM Bjorn Helgaas wrote: > > On Fri, Jun 28, 2013 at 3:51 AM, Kirill A. Shutemov > > <kirill.shutemov@linux.intel.com> wrote: > > > Bjorn Helgaas wrote: > > >> On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg > > >> <mika.westerberg@linux.intel.com> wrote: > > >> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> > > >> > > > >> > With Thunderbolt you can chain devices: connect a new devices to plugged > > >> > one. In this case the slot is already enabled, but we still want to look > > >> > for new devices behind it. > > >> > > > >> > We're going to reuse enable_device() for rescan for new devices on the > > >> > enabled slot. Let's push the check up by stack. > > >> > > > >> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > > >> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > >> > --- > > >> > drivers/pci/hotplug/acpiphp_glue.c | 5 ++--- > > >> > 1 file changed, 2 insertions(+), 3 deletions(-) > > >> > > > >> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c > > >> > index 59df857..b983e29 100644 > > >> > --- a/drivers/pci/hotplug/acpiphp_glue.c > > >> > +++ b/drivers/pci/hotplug/acpiphp_glue.c > > >> > @@ -688,9 +688,6 @@ static int __ref enable_device(struct acpiphp_slot *slot) > > >> > int num, max, pass; > > >> > LIST_HEAD(add_list); > > >> > > > >> > - if (slot->flags & SLOT_ENABLED) > > >> > - goto err_exit; > > >> > - > > >> > list_for_each_entry(func, &slot->funcs, sibling) > > >> > acpiphp_bus_add(func); > > >> > > > >> > @@ -1242,6 +1239,8 @@ int acpiphp_enable_slot(struct acpiphp_slot *slot) > > >> > goto err_exit; > > >> > > > >> > if (get_slot_status(slot) == ACPI_STA_ALL) { > > >> > + if (slot->flags & SLOT_ENABLED) > > >> > + goto err_exit; > > >> > > >> Why do we check for SLOT_ENABLED at all? I think we're handling a Bus > > >> Check notification, which means "re-enumerate on the device tree > > >> starting from the notification point." It doesn't say anything about > > >> skipping the re-enumeration if we find a device that's already > > >> enabled. > > >> > > >> It seems like we ought to just re-enumerate all the way down in case a > > >> device was added farther down in the tree (which is what it sounds > > >> like Thunderbolt is doing). > > > > > > Currently (with patchset applied), we have two users of > > > acpiphp_enable_slot(): > > > > > > - /sys/bus/pci/slots/*/power > > > - ACPI_NOTIFY_BUS_CHECK in _handle_hotplug_event_func(). > > > > > > Both are not related to Thunderbolt. > > > > > > Although, I think remove the check is good idea, I prefer to keep it > > > separate from Thunderbolt enabling patchset, since it will change sysfs > > > ABI a bit and can potentially affect othe ACPI PCI hotplug > > > implementations. > > > > I'll think about this some more, but if we can make a change that > > simplifies things and makes them more spec-compliant, and also happens > > to make Thunderbolt work, that sounds better than fixing Thunderbolt > > while leaving the wart there. > > > > If we only fix Thunderbolt, it just feels like we're adding to an > > ever-growing "deferred maintenance" list. > > I agree. > > That change may be done in a separate patch, but it should be included in the > series. Given the fact that SLOT_ENABLED is only checked in acpiphp_enable_slot() (after this patch) and that /sys/bus/pci/slots/*/power uses SLOT_POWEREDON anyway, should we remove the whole flag? -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, July 01, 2013 12:32:17 PM Mika Westerberg wrote: > On Fri, Jun 28, 2013 at 08:54:45PM +0200, Rafael J. Wysocki wrote: > > On Friday, June 28, 2013 11:00:31 AM Bjorn Helgaas wrote: > > > On Fri, Jun 28, 2013 at 3:51 AM, Kirill A. Shutemov > > > <kirill.shutemov@linux.intel.com> wrote: > > > > Bjorn Helgaas wrote: > > > >> On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg > > > >> <mika.westerberg@linux.intel.com> wrote: > > > >> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> > > > >> > > > > >> > With Thunderbolt you can chain devices: connect a new devices to plugged > > > >> > one. In this case the slot is already enabled, but we still want to look > > > >> > for new devices behind it. > > > >> > > > > >> > We're going to reuse enable_device() for rescan for new devices on the > > > >> > enabled slot. Let's push the check up by stack. > > > >> > > > > >> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > > > >> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > > >> > --- > > > >> > drivers/pci/hotplug/acpiphp_glue.c | 5 ++--- > > > >> > 1 file changed, 2 insertions(+), 3 deletions(-) > > > >> > > > > >> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c > > > >> > index 59df857..b983e29 100644 > > > >> > --- a/drivers/pci/hotplug/acpiphp_glue.c > > > >> > +++ b/drivers/pci/hotplug/acpiphp_glue.c > > > >> > @@ -688,9 +688,6 @@ static int __ref enable_device(struct acpiphp_slot *slot) > > > >> > int num, max, pass; > > > >> > LIST_HEAD(add_list); > > > >> > > > > >> > - if (slot->flags & SLOT_ENABLED) > > > >> > - goto err_exit; > > > >> > - > > > >> > list_for_each_entry(func, &slot->funcs, sibling) > > > >> > acpiphp_bus_add(func); > > > >> > > > > >> > @@ -1242,6 +1239,8 @@ int acpiphp_enable_slot(struct acpiphp_slot *slot) > > > >> > goto err_exit; > > > >> > > > > >> > if (get_slot_status(slot) == ACPI_STA_ALL) { > > > >> > + if (slot->flags & SLOT_ENABLED) > > > >> > + goto err_exit; > > > >> > > > >> Why do we check for SLOT_ENABLED at all? I think we're handling a Bus > > > >> Check notification, which means "re-enumerate on the device tree > > > >> starting from the notification point." It doesn't say anything about > > > >> skipping the re-enumeration if we find a device that's already > > > >> enabled. > > > >> > > > >> It seems like we ought to just re-enumerate all the way down in case a > > > >> device was added farther down in the tree (which is what it sounds > > > >> like Thunderbolt is doing). > > > > > > > > Currently (with patchset applied), we have two users of > > > > acpiphp_enable_slot(): > > > > > > > > - /sys/bus/pci/slots/*/power > > > > - ACPI_NOTIFY_BUS_CHECK in _handle_hotplug_event_func(). > > > > > > > > Both are not related to Thunderbolt. > > > > > > > > Although, I think remove the check is good idea, I prefer to keep it > > > > separate from Thunderbolt enabling patchset, since it will change sysfs > > > > ABI a bit and can potentially affect othe ACPI PCI hotplug > > > > implementations. > > > > > > I'll think about this some more, but if we can make a change that > > > simplifies things and makes them more spec-compliant, and also happens > > > to make Thunderbolt work, that sounds better than fixing Thunderbolt > > > while leaving the wart there. > > > > > > If we only fix Thunderbolt, it just feels like we're adding to an > > > ever-growing "deferred maintenance" list. > > > > I agree. > > > > That change may be done in a separate patch, but it should be included in the > > series. > > Given the fact that SLOT_ENABLED is only checked in acpiphp_enable_slot() > (after this patch) and that /sys/bus/pci/slots/*/power uses SLOT_POWEREDON > anyway, should we remove the whole flag? Sure, if it is not necessary any more, we should remove it. Thanks, Rafael
On Mon, Jul 01, 2013 at 04:01:37PM +0200, Rafael J. Wysocki wrote: > > Given the fact that SLOT_ENABLED is only checked in acpiphp_enable_slot() > > (after this patch) and that /sys/bus/pci/slots/*/power uses SLOT_POWEREDON > > anyway, should we remove the whole flag? > > Sure, if it is not necessary any more, we should remove it. Well, there is one thing that changes due that. Once the flag is gone userspace can do 'echo 1 > /sys/bus/pci/slots/*/power' several times and the slot is always re-enumerated. If that is not acceptable we should probably move the SLOT_ENABLED check closer to acpiphp_core:enable_device() and drop it from here, so that we always re-enumerate on Bus Check event but userspace can only do enable once (we still re-enumerate on Bus Check). -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, July 01, 2013 09:36:13 PM Mika Westerberg wrote: > On Mon, Jul 01, 2013 at 04:01:37PM +0200, Rafael J. Wysocki wrote: > > > Given the fact that SLOT_ENABLED is only checked in acpiphp_enable_slot() > > > (after this patch) and that /sys/bus/pci/slots/*/power uses SLOT_POWEREDON > > > anyway, should we remove the whole flag? > > > > Sure, if it is not necessary any more, we should remove it. > > Well, there is one thing that changes due that. Once the flag is gone > userspace can do 'echo 1 > /sys/bus/pci/slots/*/power' several times and > the slot is always re-enumerated. > > If that is not acceptable we should probably move the SLOT_ENABLED check > closer to acpiphp_core:enable_device() and drop it from here, so that we > always re-enumerate on Bus Check event but userspace can only do enable > once (we still re-enumerate on Bus Check). Yes, that sounds like the right thing to do. Thanks, Rafael
On Mon, Jul 1, 2013 at 7:29 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Monday, July 01, 2013 09:36:13 PM Mika Westerberg wrote: >> On Mon, Jul 01, 2013 at 04:01:37PM +0200, Rafael J. Wysocki wrote: >> > > Given the fact that SLOT_ENABLED is only checked in acpiphp_enable_slot() >> > > (after this patch) and that /sys/bus/pci/slots/*/power uses SLOT_POWEREDON >> > > anyway, should we remove the whole flag? >> > >> > Sure, if it is not necessary any more, we should remove it. >> >> Well, there is one thing that changes due that. Once the flag is gone >> userspace can do 'echo 1 > /sys/bus/pci/slots/*/power' several times and >> the slot is always re-enumerated. >> >> If that is not acceptable we should probably move the SLOT_ENABLED check >> closer to acpiphp_core:enable_device() and drop it from here, so that we >> always re-enumerate on Bus Check event but userspace can only do enable >> once (we still re-enumerate on Bus Check). > > Yes, that sounds like the right thing to do. Is it actually a problem if we re-enumerate every time userspace does 'echo 1 > /sys/bus/pci/slots/*/power'? I assume re-enumeration is a no-op if nothing has changed. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, July 02, 2013 10:40:39 AM Bjorn Helgaas wrote: > On Mon, Jul 1, 2013 at 7:29 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Monday, July 01, 2013 09:36:13 PM Mika Westerberg wrote: > >> On Mon, Jul 01, 2013 at 04:01:37PM +0200, Rafael J. Wysocki wrote: > >> > > Given the fact that SLOT_ENABLED is only checked in acpiphp_enable_slot() > >> > > (after this patch) and that /sys/bus/pci/slots/*/power uses SLOT_POWEREDON > >> > > anyway, should we remove the whole flag? > >> > > >> > Sure, if it is not necessary any more, we should remove it. > >> > >> Well, there is one thing that changes due that. Once the flag is gone > >> userspace can do 'echo 1 > /sys/bus/pci/slots/*/power' several times and > >> the slot is always re-enumerated. > >> > >> If that is not acceptable we should probably move the SLOT_ENABLED check > >> closer to acpiphp_core:enable_device() and drop it from here, so that we > >> always re-enumerate on Bus Check event but userspace can only do enable > >> once (we still re-enumerate on Bus Check). > > > > Yes, that sounds like the right thing to do. > > Is it actually a problem if we re-enumerate every time userspace does > 'echo 1 > /sys/bus/pci/slots/*/power'? I assume re-enumeration is a > no-op if nothing has changed. Well, if it's a no-op in that case, then re-enumerating shouldn't be a problem, but is it a no-op? Rafael (who's not really sure)
On Tue, Jul 02, 2013 at 10:29:12PM +0200, Rafael J. Wysocki wrote: > On Tuesday, July 02, 2013 10:40:39 AM Bjorn Helgaas wrote: > > On Mon, Jul 1, 2013 at 7:29 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > > On Monday, July 01, 2013 09:36:13 PM Mika Westerberg wrote: > > >> On Mon, Jul 01, 2013 at 04:01:37PM +0200, Rafael J. Wysocki wrote: > > >> > > Given the fact that SLOT_ENABLED is only checked in acpiphp_enable_slot() > > >> > > (after this patch) and that /sys/bus/pci/slots/*/power uses SLOT_POWEREDON > > >> > > anyway, should we remove the whole flag? > > >> > > > >> > Sure, if it is not necessary any more, we should remove it. > > >> > > >> Well, there is one thing that changes due that. Once the flag is gone > > >> userspace can do 'echo 1 > /sys/bus/pci/slots/*/power' several times and > > >> the slot is always re-enumerated. > > >> > > >> If that is not acceptable we should probably move the SLOT_ENABLED check > > >> closer to acpiphp_core:enable_device() and drop it from here, so that we > > >> always re-enumerate on Bus Check event but userspace can only do enable > > >> once (we still re-enumerate on Bus Check). > > > > > > Yes, that sounds like the right thing to do. > > > > Is it actually a problem if we re-enumerate every time userspace does > > 'echo 1 > /sys/bus/pci/slots/*/power'? I assume re-enumeration is a > > no-op if nothing has changed. > > Well, if it's a no-op in that case, then re-enumerating shouldn't be a problem, > but is it a no-op? I can confirm that it's a no-op (at least for the Thunderbolt case). Basically we just scan for new devices and nothing is to be found. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, July 02, 2013 11:31:17 PM Mika Westerberg wrote: > On Tue, Jul 02, 2013 at 10:29:12PM +0200, Rafael J. Wysocki wrote: > > On Tuesday, July 02, 2013 10:40:39 AM Bjorn Helgaas wrote: > > > On Mon, Jul 1, 2013 at 7:29 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > > > On Monday, July 01, 2013 09:36:13 PM Mika Westerberg wrote: > > > >> On Mon, Jul 01, 2013 at 04:01:37PM +0200, Rafael J. Wysocki wrote: > > > >> > > Given the fact that SLOT_ENABLED is only checked in acpiphp_enable_slot() > > > >> > > (after this patch) and that /sys/bus/pci/slots/*/power uses SLOT_POWEREDON > > > >> > > anyway, should we remove the whole flag? > > > >> > > > > >> > Sure, if it is not necessary any more, we should remove it. > > > >> > > > >> Well, there is one thing that changes due that. Once the flag is gone > > > >> userspace can do 'echo 1 > /sys/bus/pci/slots/*/power' several times and > > > >> the slot is always re-enumerated. > > > >> > > > >> If that is not acceptable we should probably move the SLOT_ENABLED check > > > >> closer to acpiphp_core:enable_device() and drop it from here, so that we > > > >> always re-enumerate on Bus Check event but userspace can only do enable > > > >> once (we still re-enumerate on Bus Check). > > > > > > > > Yes, that sounds like the right thing to do. > > > > > > Is it actually a problem if we re-enumerate every time userspace does > > > 'echo 1 > /sys/bus/pci/slots/*/power'? I assume re-enumeration is a > > > no-op if nothing has changed. > > > > Well, if it's a no-op in that case, then re-enumerating shouldn't be a problem, > > but is it a no-op? > > I can confirm that it's a no-op (at least for the Thunderbolt case). > Basically we just scan for new devices and nothing is to be found. We can try to drop SLOT_ENABLED entirely then I suppose. Thanks, Rafael
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c index 59df857..b983e29 100644 --- a/drivers/pci/hotplug/acpiphp_glue.c +++ b/drivers/pci/hotplug/acpiphp_glue.c @@ -688,9 +688,6 @@ static int __ref enable_device(struct acpiphp_slot *slot) int num, max, pass; LIST_HEAD(add_list); - if (slot->flags & SLOT_ENABLED) - goto err_exit; - list_for_each_entry(func, &slot->funcs, sibling) acpiphp_bus_add(func); @@ -1242,6 +1239,8 @@ int acpiphp_enable_slot(struct acpiphp_slot *slot) goto err_exit; if (get_slot_status(slot) == ACPI_STA_ALL) { + if (slot->flags & SLOT_ENABLED) + goto err_exit; /* configure all functions */ retval = enable_device(slot); if (retval)