Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755883Ab3F1RAy (ORCPT ); Fri, 28 Jun 2013 13:00:54 -0400 Received: from mail-oa0-f52.google.com ([209.85.219.52]:50685 "EHLO mail-oa0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751845Ab3F1RAw (ORCPT ); Fri, 28 Jun 2013 13:00:52 -0400 MIME-Version: 1.0 In-Reply-To: <20130628095138.D5B7BE0090@blue.fi.intel.com> References: <1372177330-28013-1-git-send-email-mika.westerberg@linux.intel.com> <1372177330-28013-2-git-send-email-mika.westerberg@linux.intel.com> <20130628095138.D5B7BE0090@blue.fi.intel.com> From: Bjorn Helgaas Date: Fri, 28 Jun 2013 11:00:31 -0600 Message-ID: Subject: Re: [PATCH 1/6] PCI: acpiphp: do not check for SLOT_ENABLED in enable_device() To: "Kirill A. Shutemov" Cc: Mika Westerberg , Greg Kroah-Hartman , "Rafael J. Wysocki" , Jesse Barnes , Yinghai Lu , "Ronciak, John" , "Penner, Miles J" , Bruce Allan , Heikki Krogerus , "linux-kernel@vger.kernel.org" , "linux-pci@vger.kernel.org" , "x86@kernel.org" Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3235 Lines: 78 On Fri, Jun 28, 2013 at 3:51 AM, Kirill A. Shutemov wrote: > Bjorn Helgaas wrote: >> On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg >> wrote: >> > From: "Kirill A. Shutemov" >> > >> > 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 >> > Signed-off-by: Mika Westerberg >> > --- >> > 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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/