[v3,3/6] ACPI/pci_slot: update PCI slot information when PCI hotplug event happens

Currently the pci_slot driver doesn't update PCI slot information
when PCI device hotplug event happens, which may cause memory leak
and returning stale information to user.

So hook the BUS_NOTIFY_ADD_DEVICE/BUS_NOTIFY_DEL_DEVICE events to
update PCI slot information when PCI hotplug event happens.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/acpi/pci_slot.c |   86 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 78 insertions(+), 8 deletions(-)

Hi,

On Wednesday, January 09, 2013 12:52:22 AM Jiang Liu wrote:
> Currently the pci_slot driver doesn't update PCI slot information
> when PCI device hotplug event happens, which may cause memory leak
> and returning stale information to user.
> 
> So hook the BUS_NOTIFY_ADD_DEVICE/BUS_NOTIFY_DEL_DEVICE events to
> update PCI slot information when PCI hotplug event happens.
> 
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
>  drivers/acpi/pci_slot.c |   86 ++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 78 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
> index d22585f..e04ea8e 100644
> --- a/drivers/acpi/pci_slot.c
> +++ b/drivers/acpi/pci_slot.c
> @@ -32,6 +32,7 @@
>  #include <linux/acpi.h>
>  #include <acpi/acpi_bus.h>
>  #include <acpi/acpi_drivers.h>
> +#include <linux/pci-acpi.h>
>  #include <linux/dmi.h>
>  
>  static bool debug;
> @@ -123,12 +124,7 @@ struct callback_args {
>  /*
>   * register_slot
>   *
> - * Called once for each SxFy object in the namespace. Don't worry about
> - * calling pci_create_slot multiple times for the same pci_bus:device,
> - * since each subsequent call simply bumps the refcount on the pci_slot.
> - *
> - * The number of calls to pci_destroy_slot from unregister_slot is
> - * symmetrical.
> + * Called once for each SxFy object in the namespace.
>   */
>  static acpi_status
>  register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
> @@ -145,6 +141,15 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
>  	if (device < 0)
>  		return AE_OK;
>  
> +	/* Avoid duplicated records for the same slot */
> +	list_for_each_entry(slot, &slot_list, list) {
> +		pci_slot = slot->pci_slot;
> +		if (pci_slot && pci_slot->bus == pci_bus &&
> +		    pci_slot->number == device) {
> +			return AE_OK;
> +		}
> +	}
> +
>  	slot = kmalloc(sizeof(*slot), GFP_KERNEL);
>  	if (!slot) {
>  		err("%s: cannot allocate memory\n", __func__);
> @@ -162,9 +167,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
>  	slot->root_handle = parent_context->root_handle;
>  	slot->pci_slot = pci_slot;
>  	INIT_LIST_HEAD(&slot->list);
> -	mutex_lock(&slot_list_lock);
>  	list_add(&slot->list, &slot_list);
> -	mutex_unlock(&slot_list_lock);
>  
>  	get_device(&pci_bus->dev);
>  
> @@ -274,7 +277,9 @@ acpi_pci_slot_add(struct acpi_pci_root *root)
>  {
>  	acpi_status status;
>  
> +	mutex_lock(&slot_list_lock);
>  	status = walk_root_bridge(root, register_slot);
> +	mutex_unlock(&slot_list_lock);
>  	if (ACPI_FAILURE(status))
>  		err("%s: register_slot failure - %d\n", __func__, status);
>  
> @@ -330,17 +335,82 @@ static struct dmi_system_id acpi_pci_slot_dmi_table[] __initdata = {
>  	{}
>  };
>  
> +static void acpi_pci_slot_notify_add(struct pci_dev *dev)
> +{
> +	acpi_handle handle;
> +	struct callback_args context;
> +
> +	if (!dev->subordinate)
> +		return;
> +
> +	mutex_lock(&slot_list_lock);
> +	handle = DEVICE_ACPI_HANDLE(&dev->dev);
> +	context.root_handle = acpi_find_root_bridge_handle(dev);

There's a patch under discussion that removes this function.

Isn't there any other way to do this?

> +	if (handle && context.root_handle) {
> +		context.pci_bus = dev->subordinate;
> +		context.user_function = register_slot;
> +		acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,

You can just pass 1 here I think.  Does the compiler complain?

> +				    register_slot, NULL, &context, NULL);
> +	}
> +	mutex_unlock(&slot_list_lock);
> +}
> +
> +static void acpi_pci_slot_notify_del(struct pci_dev *dev)
> +{
> +	struct acpi_pci_slot *slot, *tmp;
> +	struct pci_bus *bus = dev->subordinate;
> +
> +	if (!bus)
> +		return;
> +
> +	mutex_lock(&slot_list_lock);
> +	list_for_each_entry_safe(slot, tmp, &slot_list, list)
> +		if (slot->pci_slot && slot->pci_slot->bus == bus) {
> +			list_del(&slot->list);
> +			pci_destroy_slot(slot->pci_slot);
> +			put_device(&bus->dev);
> +			kfree(slot);
> +		}
> +	mutex_unlock(&slot_list_lock);
> +}
> +
> +static int acpi_pci_slot_notify_fn(struct notifier_block *nb,
> +				   unsigned long event, void *data)
> +{
> +	struct device *dev = data;
> +
> +	switch (event) {
> +	case BUS_NOTIFY_ADD_DEVICE:
> +		acpi_pci_slot_notify_add(to_pci_dev(dev));
> +		break;

Do I think correctly that this is going to be called for every PCI device
added to the system, even if it's not a bridge?

> +	case BUS_NOTIFY_DEL_DEVICE:
> +		acpi_pci_slot_notify_del(to_pci_dev(dev));
> +		break;
> +	default:
> +		return NOTIFY_DONE;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block acpi_pci_slot_notifier = {
> +	.notifier_call = &acpi_pci_slot_notify_fn,
> +};
> +
>  static int __init
>  acpi_pci_slot_init(void)
>  {
>  	dmi_check_system(acpi_pci_slot_dmi_table);
>  	acpi_pci_register_driver(&acpi_pci_slot_driver);
> +	bus_register_notifier(&pci_bus_type, &acpi_pci_slot_notifier);

I wonder if/why this has to be so convoluted?

We have found a PCI bridge in the ACPI namespace, so we've created a struct
acpi_device for it and we've walked the namespace below it already.

Now we're creating a struct pci_dev for it and while registering it we're
going to walk the namespace below the bridge again to find and register its
slots and that is done indirectly from a bus type notifier.

Why can't we enumerate the slots directly upfront?

> +
>  	return 0;
>  }
>  
>  static void __exit
>  acpi_pci_slot_exit(void)
>  {
> +	bus_unregister_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
>  	acpi_pci_unregister_driver(&acpi_pci_slot_driver);
>  }

Thanks,
Rafael

Hi Rafael,
	Thanks for your great efforts to review the patch.	

On 01/09/2013 08:01 AM, Rafael J. Wysocki wrote:
> Hi,
> 
> On Wednesday, January 09, 2013 12:52:22 AM Jiang Liu wrote:
snip
>>  
>> +static void acpi_pci_slot_notify_add(struct pci_dev *dev)
>> +{
>> +	acpi_handle handle;
>> +	struct callback_args context;
>> +
>> +	if (!dev->subordinate)
>> +		return;
>> +
>> +	mutex_lock(&slot_list_lock);
>> +	handle = DEVICE_ACPI_HANDLE(&dev->dev);
>> +	context.root_handle = acpi_find_root_bridge_handle(dev);
> 
> There's a patch under discussion that removes this function.
> 
> Isn't there any other way to do this?
	I will try to find a way to get rid of calling acpi_find_root_bridge_handle,
and it seems doable.

> 
>> +	if (handle && context.root_handle) {
>> +		context.pci_bus = dev->subordinate;
>> +		context.user_function = register_slot;
>> +		acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
> 
> You can just pass 1 here I think.  Does the compiler complain?
Thanks for reminder, the (u32) is unnecessary.

> 
>> +				    register_slot, NULL, &context, NULL);
>> +	}
>> +	mutex_unlock(&slot_list_lock);
>> +}
>> +
>> +static void acpi_pci_slot_notify_del(struct pci_dev *dev)
>> +{
>> +	struct acpi_pci_slot *slot, *tmp;
>> +	struct pci_bus *bus = dev->subordinate;
>> +
>> +	if (!bus)
>> +		return;
>> +
>> +	mutex_lock(&slot_list_lock);
>> +	list_for_each_entry_safe(slot, tmp, &slot_list, list)
>> +		if (slot->pci_slot && slot->pci_slot->bus == bus) {
>> +			list_del(&slot->list);
>> +			pci_destroy_slot(slot->pci_slot);
>> +			put_device(&bus->dev);
>> +			kfree(slot);
>> +		}
>> +	mutex_unlock(&slot_list_lock);
>> +}
>> +
>> +static int acpi_pci_slot_notify_fn(struct notifier_block *nb,
>> +				   unsigned long event, void *data)
>> +{
>> +	struct device *dev = data;
>> +
>> +	switch (event) {
>> +	case BUS_NOTIFY_ADD_DEVICE:
>> +		acpi_pci_slot_notify_add(to_pci_dev(dev));
>> +		break;
> 
> Do I think correctly that this is going to be called for every PCI device
> added to the system, even if it's not a bridge?
You are right. Function acpi_pci_slot_notify_add() and acpi_pci_slot_notify_del()
will check whether it's a bridge. If preferred, I will move the check up into
acpi_pci_slot_notify_fn().

> 
>> +	case BUS_NOTIFY_DEL_DEVICE:
>> +		acpi_pci_slot_notify_del(to_pci_dev(dev));
>> +		break;
>> +	default:
>> +		return NOTIFY_DONE;
>> +	}
>> +
>> +	return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block acpi_pci_slot_notifier = {
>> +	.notifier_call = &acpi_pci_slot_notify_fn,
>> +};
>> +
>>  static int __init
>>  acpi_pci_slot_init(void)
>>  {
>>  	dmi_check_system(acpi_pci_slot_dmi_table);
>>  	acpi_pci_register_driver(&acpi_pci_slot_driver);
>> +	bus_register_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
> 
> I wonder if/why this has to be so convoluted?
> 
> We have found a PCI bridge in the ACPI namespace, so we've created a struct
> acpi_device for it and we've walked the namespace below it already.
> 
> Now we're creating a struct pci_dev for it and while registering it we're
> going to walk the namespace below the bridge again to find and register its
> slots and that is done indirectly from a bus type notifier.
> 
> Why can't we enumerate the slots directly upfront?
Do you mean to create the PCI slot devices when creating the ACPI devices?
I think there are two factors prevent us from doing that.
The first is that the ACPI pci_slot driver could be built as a module, so
we can't call into it from the ACPI core.
The second reason is that the PCI slot is associated with a PCI bus, and the
bus is only available until the PCI device has been created.

Thanks!
Gerry
> 
>> +
>>  	return 0;
>>  }
>>  
>>  static void __exit
>>  acpi_pci_slot_exit(void)
>>  {
>> +	bus_unregister_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
>>  	acpi_pci_unregister_driver(&acpi_pci_slot_driver);
>>  }
> 
> Thanks,
> Rafael
> 
> 

--
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 Thursday, January 10, 2013 12:58:25 AM Jiang Liu wrote:
> Hi Rafael,
> 	Thanks for your great efforts to review the patch.	
> 
> On 01/09/2013 08:01 AM, Rafael J. Wysocki wrote:
> > Hi,
> > 
> > On Wednesday, January 09, 2013 12:52:22 AM Jiang Liu wrote:
> snip
> >>  
> >> +static void acpi_pci_slot_notify_add(struct pci_dev *dev)
> >> +{
> >> +	acpi_handle handle;
> >> +	struct callback_args context;
> >> +
> >> +	if (!dev->subordinate)
> >> +		return;
> >> +
> >> +	mutex_lock(&slot_list_lock);
> >> +	handle = DEVICE_ACPI_HANDLE(&dev->dev);
> >> +	context.root_handle = acpi_find_root_bridge_handle(dev);
> > 
> > There's a patch under discussion that removes this function.
> > 
> > Isn't there any other way to do this?
> 	I will try to find a way to get rid of calling acpi_find_root_bridge_handle,
> and it seems doable.
> 
> > 
> >> +	if (handle && context.root_handle) {
> >> +		context.pci_bus = dev->subordinate;
> >> +		context.user_function = register_slot;
> >> +		acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
> > 
> > You can just pass 1 here I think.  Does the compiler complain?
> Thanks for reminder, the (u32) is unnecessary.
> 
> > 
> >> +				    register_slot, NULL, &context, NULL);
> >> +	}
> >> +	mutex_unlock(&slot_list_lock);
> >> +}
> >> +
> >> +static void acpi_pci_slot_notify_del(struct pci_dev *dev)
> >> +{
> >> +	struct acpi_pci_slot *slot, *tmp;
> >> +	struct pci_bus *bus = dev->subordinate;
> >> +
> >> +	if (!bus)
> >> +		return;
> >> +
> >> +	mutex_lock(&slot_list_lock);
> >> +	list_for_each_entry_safe(slot, tmp, &slot_list, list)
> >> +		if (slot->pci_slot && slot->pci_slot->bus == bus) {
> >> +			list_del(&slot->list);
> >> +			pci_destroy_slot(slot->pci_slot);
> >> +			put_device(&bus->dev);
> >> +			kfree(slot);
> >> +		}
> >> +	mutex_unlock(&slot_list_lock);
> >> +}
> >> +
> >> +static int acpi_pci_slot_notify_fn(struct notifier_block *nb,
> >> +				   unsigned long event, void *data)
> >> +{
> >> +	struct device *dev = data;
> >> +
> >> +	switch (event) {
> >> +	case BUS_NOTIFY_ADD_DEVICE:
> >> +		acpi_pci_slot_notify_add(to_pci_dev(dev));
> >> +		break;
> > 
> > Do I think correctly that this is going to be called for every PCI device
> > added to the system, even if it's not a bridge?
> You are right. Function acpi_pci_slot_notify_add() and acpi_pci_slot_notify_del()
> will check whether it's a bridge. If preferred, I will move the check up into
> acpi_pci_slot_notify_fn().
> 
> > 
> >> +	case BUS_NOTIFY_DEL_DEVICE:
> >> +		acpi_pci_slot_notify_del(to_pci_dev(dev));
> >> +		break;
> >> +	default:
> >> +		return NOTIFY_DONE;
> >> +	}
> >> +
> >> +	return NOTIFY_OK;
> >> +}
> >> +
> >> +static struct notifier_block acpi_pci_slot_notifier = {
> >> +	.notifier_call = &acpi_pci_slot_notify_fn,
> >> +};
> >> +
> >>  static int __init
> >>  acpi_pci_slot_init(void)
> >>  {
> >>  	dmi_check_system(acpi_pci_slot_dmi_table);
> >>  	acpi_pci_register_driver(&acpi_pci_slot_driver);
> >> +	bus_register_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
> > 
> > I wonder if/why this has to be so convoluted?
> > 
> > We have found a PCI bridge in the ACPI namespace, so we've created a struct
> > acpi_device for it and we've walked the namespace below it already.
> > 
> > Now we're creating a struct pci_dev for it and while registering it we're
> > going to walk the namespace below the bridge again to find and register its
> > slots and that is done indirectly from a bus type notifier.
> > 
> > Why can't we enumerate the slots directly upfront?
> Do you mean to create the PCI slot devices when creating the ACPI devices?
> I think there are two factors prevent us from doing that.
> The first is that the ACPI pci_slot driver could be built as a module, so
> we can't call into it from the ACPI core.

I didn't say about calling the pci_slot driver from the ACPI core, but about
enumerating slots in a way suitable for consumption by the pci_slot driver
when it's ready.

That said I really don't see a value in having a modular pci_slot driver.  It
is part of the hotplug infrastructure and should always be presend for this
reason, so we don't need to worry about the "pci_slot driver not present" case.

> The second reason is that the PCI slot is associated with a PCI bus, and the
> bus is only available until the PCI device has been created.

I suppose you mean "after"?  [I'm not sure if I agree with that, but whatever.]

We know which devices have slots before that happens, though.

Thanks,
Rafael

[+cc Myron]

On Wed, Jan 9, 2013 at 1:19 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, January 10, 2013 12:58:25 AM Jiang Liu wrote:
>> Hi Rafael,
>>       Thanks for your great efforts to review the patch.
>>
>> On 01/09/2013 08:01 AM, Rafael J. Wysocki wrote:
>> > Hi,
>> >
>> > On Wednesday, January 09, 2013 12:52:22 AM Jiang Liu wrote:
>> snip
>> >>
>> >> +static void acpi_pci_slot_notify_add(struct pci_dev *dev)
>> >> +{
>> >> +  acpi_handle handle;
>> >> +  struct callback_args context;
>> >> +
>> >> +  if (!dev->subordinate)
>> >> +          return;
>> >> +
>> >> +  mutex_lock(&slot_list_lock);
>> >> +  handle = DEVICE_ACPI_HANDLE(&dev->dev);
>> >> +  context.root_handle = acpi_find_root_bridge_handle(dev);
>> >
>> > There's a patch under discussion that removes this function.
>> >
>> > Isn't there any other way to do this?
>>       I will try to find a way to get rid of calling acpi_find_root_bridge_handle,
>> and it seems doable.
>>
>> >
>> >> +  if (handle && context.root_handle) {
>> >> +          context.pci_bus = dev->subordinate;
>> >> +          context.user_function = register_slot;
>> >> +          acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
>> >
>> > You can just pass 1 here I think.  Does the compiler complain?
>> Thanks for reminder, the (u32) is unnecessary.
>>
>> >
>> >> +                              register_slot, NULL, &context, NULL);
>> >> +  }
>> >> +  mutex_unlock(&slot_list_lock);
>> >> +}
>> >> +
>> >> +static void acpi_pci_slot_notify_del(struct pci_dev *dev)
>> >> +{
>> >> +  struct acpi_pci_slot *slot, *tmp;
>> >> +  struct pci_bus *bus = dev->subordinate;
>> >> +
>> >> +  if (!bus)
>> >> +          return;
>> >> +
>> >> +  mutex_lock(&slot_list_lock);
>> >> +  list_for_each_entry_safe(slot, tmp, &slot_list, list)
>> >> +          if (slot->pci_slot && slot->pci_slot->bus == bus) {
>> >> +                  list_del(&slot->list);
>> >> +                  pci_destroy_slot(slot->pci_slot);
>> >> +                  put_device(&bus->dev);
>> >> +                  kfree(slot);
>> >> +          }
>> >> +  mutex_unlock(&slot_list_lock);
>> >> +}
>> >> +
>> >> +static int acpi_pci_slot_notify_fn(struct notifier_block *nb,
>> >> +                             unsigned long event, void *data)
>> >> +{
>> >> +  struct device *dev = data;
>> >> +
>> >> +  switch (event) {
>> >> +  case BUS_NOTIFY_ADD_DEVICE:
>> >> +          acpi_pci_slot_notify_add(to_pci_dev(dev));
>> >> +          break;
>> >
>> > Do I think correctly that this is going to be called for every PCI device
>> > added to the system, even if it's not a bridge?
>> You are right. Function acpi_pci_slot_notify_add() and acpi_pci_slot_notify_del()
>> will check whether it's a bridge. If preferred, I will move the check up into
>> acpi_pci_slot_notify_fn().
>>
>> >
>> >> +  case BUS_NOTIFY_DEL_DEVICE:
>> >> +          acpi_pci_slot_notify_del(to_pci_dev(dev));
>> >> +          break;
>> >> +  default:
>> >> +          return NOTIFY_DONE;
>> >> +  }
>> >> +
>> >> +  return NOTIFY_OK;
>> >> +}
>> >> +
>> >> +static struct notifier_block acpi_pci_slot_notifier = {
>> >> +  .notifier_call = &acpi_pci_slot_notify_fn,
>> >> +};
>> >> +
>> >>  static int __init
>> >>  acpi_pci_slot_init(void)
>> >>  {
>> >>    dmi_check_system(acpi_pci_slot_dmi_table);
>> >>    acpi_pci_register_driver(&acpi_pci_slot_driver);
>> >> +  bus_register_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
>> >
>> > I wonder if/why this has to be so convoluted?
>> >
>> > We have found a PCI bridge in the ACPI namespace, so we've created a struct
>> > acpi_device for it and we've walked the namespace below it already.
>> >
>> > Now we're creating a struct pci_dev for it and while registering it we're
>> > going to walk the namespace below the bridge again to find and register its
>> > slots and that is done indirectly from a bus type notifier.
>> >
>> > Why can't we enumerate the slots directly upfront?
>> Do you mean to create the PCI slot devices when creating the ACPI devices?
>> I think there are two factors prevent us from doing that.
>> The first is that the ACPI pci_slot driver could be built as a module, so
>> we can't call into it from the ACPI core.
>
> I didn't say about calling the pci_slot driver from the ACPI core, but about
> enumerating slots in a way suitable for consumption by the pci_slot driver
> when it's ready.
>
> That said I really don't see a value in having a modular pci_slot driver.  It
> is part of the hotplug infrastructure and should always be presend for this
> reason, so we don't need to worry about the "pci_slot driver not present" case.

I agree that there's no value in supporting CONFIG_ACPI_PCI_SLOT=m.  I
think Myron has some patches that remove that case.

I'm not sure what the best way to merge them is.  We have a bunch of
stuff this cycle that touches both ACPI and PCI.

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 Wednesday, January 09, 2013 01:44:23 PM Bjorn Helgaas wrote:
> [+cc Myron]
> 
> On Wed, Jan 9, 2013 at 1:19 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday, January 10, 2013 12:58:25 AM Jiang Liu wrote:
> >> Hi Rafael,
> >>       Thanks for your great efforts to review the patch.
> >>
> >> On 01/09/2013 08:01 AM, Rafael J. Wysocki wrote:
> >> > Hi,
> >> >
> >> > On Wednesday, January 09, 2013 12:52:22 AM Jiang Liu wrote:
> >> snip
> >> >>
> >> >> +static void acpi_pci_slot_notify_add(struct pci_dev *dev)
> >> >> +{
> >> >> +  acpi_handle handle;
> >> >> +  struct callback_args context;
> >> >> +
> >> >> +  if (!dev->subordinate)
> >> >> +          return;
> >> >> +
> >> >> +  mutex_lock(&slot_list_lock);
> >> >> +  handle = DEVICE_ACPI_HANDLE(&dev->dev);
> >> >> +  context.root_handle = acpi_find_root_bridge_handle(dev);
> >> >
> >> > There's a patch under discussion that removes this function.
> >> >
> >> > Isn't there any other way to do this?
> >>       I will try to find a way to get rid of calling acpi_find_root_bridge_handle,
> >> and it seems doable.
> >>
> >> >
> >> >> +  if (handle && context.root_handle) {
> >> >> +          context.pci_bus = dev->subordinate;
> >> >> +          context.user_function = register_slot;
> >> >> +          acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
> >> >
> >> > You can just pass 1 here I think.  Does the compiler complain?
> >> Thanks for reminder, the (u32) is unnecessary.
> >>
> >> >
> >> >> +                              register_slot, NULL, &context, NULL);
> >> >> +  }
> >> >> +  mutex_unlock(&slot_list_lock);
> >> >> +}
> >> >> +
> >> >> +static void acpi_pci_slot_notify_del(struct pci_dev *dev)
> >> >> +{
> >> >> +  struct acpi_pci_slot *slot, *tmp;
> >> >> +  struct pci_bus *bus = dev->subordinate;
> >> >> +
> >> >> +  if (!bus)
> >> >> +          return;
> >> >> +
> >> >> +  mutex_lock(&slot_list_lock);
> >> >> +  list_for_each_entry_safe(slot, tmp, &slot_list, list)
> >> >> +          if (slot->pci_slot && slot->pci_slot->bus == bus) {
> >> >> +                  list_del(&slot->list);
> >> >> +                  pci_destroy_slot(slot->pci_slot);
> >> >> +                  put_device(&bus->dev);
> >> >> +                  kfree(slot);
> >> >> +          }
> >> >> +  mutex_unlock(&slot_list_lock);
> >> >> +}
> >> >> +
> >> >> +static int acpi_pci_slot_notify_fn(struct notifier_block *nb,
> >> >> +                             unsigned long event, void *data)
> >> >> +{
> >> >> +  struct device *dev = data;
> >> >> +
> >> >> +  switch (event) {
> >> >> +  case BUS_NOTIFY_ADD_DEVICE:
> >> >> +          acpi_pci_slot_notify_add(to_pci_dev(dev));
> >> >> +          break;
> >> >
> >> > Do I think correctly that this is going to be called for every PCI device
> >> > added to the system, even if it's not a bridge?
> >> You are right. Function acpi_pci_slot_notify_add() and acpi_pci_slot_notify_del()
> >> will check whether it's a bridge. If preferred, I will move the check up into
> >> acpi_pci_slot_notify_fn().
> >>
> >> >
> >> >> +  case BUS_NOTIFY_DEL_DEVICE:
> >> >> +          acpi_pci_slot_notify_del(to_pci_dev(dev));
> >> >> +          break;
> >> >> +  default:
> >> >> +          return NOTIFY_DONE;
> >> >> +  }
> >> >> +
> >> >> +  return NOTIFY_OK;
> >> >> +}
> >> >> +
> >> >> +static struct notifier_block acpi_pci_slot_notifier = {
> >> >> +  .notifier_call = &acpi_pci_slot_notify_fn,
> >> >> +};
> >> >> +
> >> >>  static int __init
> >> >>  acpi_pci_slot_init(void)
> >> >>  {
> >> >>    dmi_check_system(acpi_pci_slot_dmi_table);
> >> >>    acpi_pci_register_driver(&acpi_pci_slot_driver);
> >> >> +  bus_register_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
> >> >
> >> > I wonder if/why this has to be so convoluted?
> >> >
> >> > We have found a PCI bridge in the ACPI namespace, so we've created a struct
> >> > acpi_device for it and we've walked the namespace below it already.
> >> >
> >> > Now we're creating a struct pci_dev for it and while registering it we're
> >> > going to walk the namespace below the bridge again to find and register its
> >> > slots and that is done indirectly from a bus type notifier.
> >> >
> >> > Why can't we enumerate the slots directly upfront?
> >> Do you mean to create the PCI slot devices when creating the ACPI devices?
> >> I think there are two factors prevent us from doing that.
> >> The first is that the ACPI pci_slot driver could be built as a module, so
> >> we can't call into it from the ACPI core.
> >
> > I didn't say about calling the pci_slot driver from the ACPI core, but about
> > enumerating slots in a way suitable for consumption by the pci_slot driver
> > when it's ready.
> >
> > That said I really don't see a value in having a modular pci_slot driver.  It
> > is part of the hotplug infrastructure and should always be presend for this
> > reason, so we don't need to worry about the "pci_slot driver not present" case.
> 
> I agree that there's no value in supporting CONFIG_ACPI_PCI_SLOT=m.  I
> think Myron has some patches that remove that case.
> 
> I'm not sure what the best way to merge them is.  We have a bunch of
> stuff this cycle that touches both ACPI and PCI.

Well, is there a possibility to pull my acpi-scan branch into your tree?

Rafael

[+cc Yinghai]

On Wed, 2013-01-09 at 13:44 -0700, Bjorn Helgaas wrote:
> [+cc Myron]
> 
> On Wed, Jan 9, 2013 at 1:19 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday, January 10, 2013 12:58:25 AM Jiang Liu wrote:
> >> Hi Rafael,
> >>       Thanks for your great efforts to review the patch.
> >>
> >> On 01/09/2013 08:01 AM, Rafael J. Wysocki wrote:
> >> > Hi,
> >> >
> >> > On Wednesday, January 09, 2013 12:52:22 AM Jiang Liu wrote:
> >> snip
> >> >>
> >> >> +static void acpi_pci_slot_notify_add(struct pci_dev *dev)
> >> >> +{
> >> >> +  acpi_handle handle;
> >> >> +  struct callback_args context;
> >> >> +
> >> >> +  if (!dev->subordinate)
> >> >> +          return;
> >> >> +
> >> >> +  mutex_lock(&slot_list_lock);
> >> >> +  handle = DEVICE_ACPI_HANDLE(&dev->dev);
> >> >> +  context.root_handle = acpi_find_root_bridge_handle(dev);
> >> >
> >> > There's a patch under discussion that removes this function.
> >> >
> >> > Isn't there any other way to do this?
> >>       I will try to find a way to get rid of calling acpi_find_root_bridge_handle,
> >> and it seems doable.
> >>
> >> >
> >> >> +  if (handle && context.root_handle) {
> >> >> +          context.pci_bus = dev->subordinate;
> >> >> +          context.user_function = register_slot;
> >> >> +          acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
> >> >
> >> > You can just pass 1 here I think.  Does the compiler complain?
> >> Thanks for reminder, the (u32) is unnecessary.
> >>
> >> >
> >> >> +                              register_slot, NULL, &context, NULL);
> >> >> +  }
> >> >> +  mutex_unlock(&slot_list_lock);
> >> >> +}
> >> >> +
> >> >> +static void acpi_pci_slot_notify_del(struct pci_dev *dev)
> >> >> +{
> >> >> +  struct acpi_pci_slot *slot, *tmp;
> >> >> +  struct pci_bus *bus = dev->subordinate;
> >> >> +
> >> >> +  if (!bus)
> >> >> +          return;
> >> >> +
> >> >> +  mutex_lock(&slot_list_lock);
> >> >> +  list_for_each_entry_safe(slot, tmp, &slot_list, list)
> >> >> +          if (slot->pci_slot && slot->pci_slot->bus == bus) {
> >> >> +                  list_del(&slot->list);
> >> >> +                  pci_destroy_slot(slot->pci_slot);
> >> >> +                  put_device(&bus->dev);
> >> >> +                  kfree(slot);
> >> >> +          }
> >> >> +  mutex_unlock(&slot_list_lock);
> >> >> +}
> >> >> +
> >> >> +static int acpi_pci_slot_notify_fn(struct notifier_block *nb,
> >> >> +                             unsigned long event, void *data)
> >> >> +{
> >> >> +  struct device *dev = data;
> >> >> +
> >> >> +  switch (event) {
> >> >> +  case BUS_NOTIFY_ADD_DEVICE:
> >> >> +          acpi_pci_slot_notify_add(to_pci_dev(dev));
> >> >> +          break;
> >> >
> >> > Do I think correctly that this is going to be called for every PCI device
> >> > added to the system, even if it's not a bridge?
> >> You are right. Function acpi_pci_slot_notify_add() and acpi_pci_slot_notify_del()
> >> will check whether it's a bridge. If preferred, I will move the check up into
> >> acpi_pci_slot_notify_fn().
> >>
> >> >
> >> >> +  case BUS_NOTIFY_DEL_DEVICE:
> >> >> +          acpi_pci_slot_notify_del(to_pci_dev(dev));
> >> >> +          break;
> >> >> +  default:
> >> >> +          return NOTIFY_DONE;
> >> >> +  }
> >> >> +
> >> >> +  return NOTIFY_OK;
> >> >> +}
> >> >> +
> >> >> +static struct notifier_block acpi_pci_slot_notifier = {
> >> >> +  .notifier_call = &acpi_pci_slot_notify_fn,
> >> >> +};
> >> >> +
> >> >>  static int __init
> >> >>  acpi_pci_slot_init(void)
> >> >>  {
> >> >>    dmi_check_system(acpi_pci_slot_dmi_table);
> >> >>    acpi_pci_register_driver(&acpi_pci_slot_driver);
> >> >> +  bus_register_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
> >> >
> >> > I wonder if/why this has to be so convoluted?
> >> >
> >> > We have found a PCI bridge in the ACPI namespace, so we've created a struct
> >> > acpi_device for it and we've walked the namespace below it already.
> >> >
> >> > Now we're creating a struct pci_dev for it and while registering it we're
> >> > going to walk the namespace below the bridge again to find and register its
> >> > slots and that is done indirectly from a bus type notifier.
> >> >
> >> > Why can't we enumerate the slots directly upfront?
> >> Do you mean to create the PCI slot devices when creating the ACPI devices?
> >> I think there are two factors prevent us from doing that.
> >> The first is that the ACPI pci_slot driver could be built as a module, so
> >> we can't call into it from the ACPI core.
> >
> > I didn't say about calling the pci_slot driver from the ACPI core, but about
> > enumerating slots in a way suitable for consumption by the pci_slot driver
> > when it's ready.
> >
> > That said I really don't see a value in having a modular pci_slot driver.  It
> > is part of the hotplug infrastructure and should always be presend for this
> > reason, so we don't need to worry about the "pci_slot driver not present" case.
> 
> I agree that there's no value in supporting CONFIG_ACPI_PCI_SLOT=m.  I
> think Myron has some patches that remove that case.
> 
> I'm not sure what the best way to merge them is.  We have a bunch of
> stuff this cycle that touches both ACPI and PCI.

Rafael:

The series Bjorn mentions is at https://lkml.org/lkml/2012/12/7/11  It
converts both the "ACPI Hot Plug PCI Controller Driver ("acpiphp")" and
"ACPI PCI Slot Detection Driver ("pci_slot")" sub-drivers to built-in
drivers (i.e. no longer supported as modules).

Yinghai commented back - https://lkml.org/lkml/2012/12/7/29 - indicating
a possible issue with such but his response was too terse for me to get
anything from.  Also, I've noticed that a couple of the distros, debian
being one, have made similar changes so I'm a little skeptical about
Yinghai's concerns.

Since you have similar thoughts - "I really don't see a value in having
a modular pci_slot driver" - can you foresee any issues with such?

Thanks,
 Myron
> 
> 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 Thursday, January 10, 2013 02:24:23 PM Myron Stowe wrote:
> [+cc Yinghai]
> 
> On Wed, 2013-01-09 at 13:44 -0700, Bjorn Helgaas wrote:
> > [+cc Myron]
> > 
> > On Wed, Jan 9, 2013 at 1:19 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > On Thursday, January 10, 2013 12:58:25 AM Jiang Liu wrote:
> > >> Hi Rafael,
> > >>       Thanks for your great efforts to review the patch.
> > >>
> > >> On 01/09/2013 08:01 AM, Rafael J. Wysocki wrote:
> > >> > Hi,
> > >> >
> > >> > On Wednesday, January 09, 2013 12:52:22 AM Jiang Liu wrote:
> > >> snip
> > >> >>
> > >> >> +static void acpi_pci_slot_notify_add(struct pci_dev *dev)
> > >> >> +{
> > >> >> +  acpi_handle handle;
> > >> >> +  struct callback_args context;
> > >> >> +
> > >> >> +  if (!dev->subordinate)
> > >> >> +          return;
> > >> >> +
> > >> >> +  mutex_lock(&slot_list_lock);
> > >> >> +  handle = DEVICE_ACPI_HANDLE(&dev->dev);
> > >> >> +  context.root_handle = acpi_find_root_bridge_handle(dev);
> > >> >
> > >> > There's a patch under discussion that removes this function.
> > >> >
> > >> > Isn't there any other way to do this?
> > >>       I will try to find a way to get rid of calling acpi_find_root_bridge_handle,
> > >> and it seems doable.
> > >>
> > >> >
> > >> >> +  if (handle && context.root_handle) {
> > >> >> +          context.pci_bus = dev->subordinate;
> > >> >> +          context.user_function = register_slot;
> > >> >> +          acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
> > >> >
> > >> > You can just pass 1 here I think.  Does the compiler complain?
> > >> Thanks for reminder, the (u32) is unnecessary.
> > >>
> > >> >
> > >> >> +                              register_slot, NULL, &context, NULL);
> > >> >> +  }
> > >> >> +  mutex_unlock(&slot_list_lock);
> > >> >> +}
> > >> >> +
> > >> >> +static void acpi_pci_slot_notify_del(struct pci_dev *dev)
> > >> >> +{
> > >> >> +  struct acpi_pci_slot *slot, *tmp;
> > >> >> +  struct pci_bus *bus = dev->subordinate;
> > >> >> +
> > >> >> +  if (!bus)
> > >> >> +          return;
> > >> >> +
> > >> >> +  mutex_lock(&slot_list_lock);
> > >> >> +  list_for_each_entry_safe(slot, tmp, &slot_list, list)
> > >> >> +          if (slot->pci_slot && slot->pci_slot->bus == bus) {
> > >> >> +                  list_del(&slot->list);
> > >> >> +                  pci_destroy_slot(slot->pci_slot);
> > >> >> +                  put_device(&bus->dev);
> > >> >> +                  kfree(slot);
> > >> >> +          }
> > >> >> +  mutex_unlock(&slot_list_lock);
> > >> >> +}
> > >> >> +
> > >> >> +static int acpi_pci_slot_notify_fn(struct notifier_block *nb,
> > >> >> +                             unsigned long event, void *data)
> > >> >> +{
> > >> >> +  struct device *dev = data;
> > >> >> +
> > >> >> +  switch (event) {
> > >> >> +  case BUS_NOTIFY_ADD_DEVICE:
> > >> >> +          acpi_pci_slot_notify_add(to_pci_dev(dev));
> > >> >> +          break;
> > >> >
> > >> > Do I think correctly that this is going to be called for every PCI device
> > >> > added to the system, even if it's not a bridge?
> > >> You are right. Function acpi_pci_slot_notify_add() and acpi_pci_slot_notify_del()
> > >> will check whether it's a bridge. If preferred, I will move the check up into
> > >> acpi_pci_slot_notify_fn().
> > >>
> > >> >
> > >> >> +  case BUS_NOTIFY_DEL_DEVICE:
> > >> >> +          acpi_pci_slot_notify_del(to_pci_dev(dev));
> > >> >> +          break;
> > >> >> +  default:
> > >> >> +          return NOTIFY_DONE;
> > >> >> +  }
> > >> >> +
> > >> >> +  return NOTIFY_OK;
> > >> >> +}
> > >> >> +
> > >> >> +static struct notifier_block acpi_pci_slot_notifier = {
> > >> >> +  .notifier_call = &acpi_pci_slot_notify_fn,
> > >> >> +};
> > >> >> +
> > >> >>  static int __init
> > >> >>  acpi_pci_slot_init(void)
> > >> >>  {
> > >> >>    dmi_check_system(acpi_pci_slot_dmi_table);
> > >> >>    acpi_pci_register_driver(&acpi_pci_slot_driver);
> > >> >> +  bus_register_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
> > >> >
> > >> > I wonder if/why this has to be so convoluted?
> > >> >
> > >> > We have found a PCI bridge in the ACPI namespace, so we've created a struct
> > >> > acpi_device for it and we've walked the namespace below it already.
> > >> >
> > >> > Now we're creating a struct pci_dev for it and while registering it we're
> > >> > going to walk the namespace below the bridge again to find and register its
> > >> > slots and that is done indirectly from a bus type notifier.
> > >> >
> > >> > Why can't we enumerate the slots directly upfront?
> > >> Do you mean to create the PCI slot devices when creating the ACPI devices?
> > >> I think there are two factors prevent us from doing that.
> > >> The first is that the ACPI pci_slot driver could be built as a module, so
> > >> we can't call into it from the ACPI core.
> > >
> > > I didn't say about calling the pci_slot driver from the ACPI core, but about
> > > enumerating slots in a way suitable for consumption by the pci_slot driver
> > > when it's ready.
> > >
> > > That said I really don't see a value in having a modular pci_slot driver.  It
> > > is part of the hotplug infrastructure and should always be presend for this
> > > reason, so we don't need to worry about the "pci_slot driver not present" case.
> > 
> > I agree that there's no value in supporting CONFIG_ACPI_PCI_SLOT=m.  I
> > think Myron has some patches that remove that case.
> > 
> > I'm not sure what the best way to merge them is.  We have a bunch of
> > stuff this cycle that touches both ACPI and PCI.
> 
> Rafael:
> 
> The series Bjorn mentions is at https://lkml.org/lkml/2012/12/7/11  It
> converts both the "ACPI Hot Plug PCI Controller Driver ("acpiphp")" and
> "ACPI PCI Slot Detection Driver ("pci_slot")" sub-drivers to built-in
> drivers (i.e. no longer supported as modules).
> 
> Yinghai commented back - https://lkml.org/lkml/2012/12/7/29 - indicating
> a possible issue with such but his response was too terse for me to get
> anything from.  Also, I've noticed that a couple of the distros, debian
> being one, have made similar changes so I'm a little skeptical about
> Yinghai's concerns.
> 
> Since you have similar thoughts - "I really don't see a value in having
> a modular pci_slot driver" - can you foresee any issues with such?

Well, I don't see what functional problems that can bring.

In theory people may want to have them as modules to avoid loading them on
systems that don't use PCI hotplug, but honestly I think that the complexity
this causes us to deal with is not worth it.

Moreover, removing the modularity may actually allow us to solve some ordering
issues once and for good.

Thanks,
Rafael

On Thu, Jan 10, 2013 at 1:50 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> Well, I don't see what functional problems that can bring.
>
> In theory people may want to have them as modules to avoid loading them on
> systems that don't use PCI hotplug, but honestly I think that the complexity
> this causes us to deal with is not worth it.
>
> Moreover, removing the modularity may actually allow us to solve some ordering
> issues once and for good.

No, the world is not really ideal yet.

looks like laptops have problem with pci express cards.

when pciehp is used, surprise insert/removal does not work because
PresDect does not change properly, so no interrupt is generated.
--- i suspects that is silicon problem.

but when acpiphp is used, that surprise  insert/removal is working.

some laptop like thinkpad, just don't give osc to kernel..
[    0.505117]  pci0000:00: Requesting ACPI _OSC control (0x1d)
[    0.505413]  pci0000:00: ACPI _OSC request failed (AE_SUPPORT),
returned control mask: 0x0d
[    0.505517] ACPI _OSC control for PCIe not granted, disabling ASPM

and other laptop give that to kernel, in recent kernel will not give
acpiphp to have that slot, because it want to hold that for pciehp.
poor user have to pass 'pci_aspm=off" to disable _OSC for all.
--- please check the mail that i forward to you yesterday.

Anyway, we do need to let the user to have choice to use acpiphp and pciehp.
and it should be first come and first serve policy.

Thanks

Yinghai
--
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 Thursday, January 10, 2013 03:03:53 PM Yinghai Lu wrote:
> On Thu, Jan 10, 2013 at 1:50 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > Well, I don't see what functional problems that can bring.
> >
> > In theory people may want to have them as modules to avoid loading them on
> > systems that don't use PCI hotplug, but honestly I think that the complexity
> > this causes us to deal with is not worth it.
> >
> > Moreover, removing the modularity may actually allow us to solve some ordering
> > issues once and for good.
> 
> No, the world is not really ideal yet.
> 
> looks like laptops have problem with pci express cards.
> 
> when pciehp is used, surprise insert/removal does not work because
> PresDect does not change properly, so no interrupt is generated.
> --- i suspects that is silicon problem.
> 
> but when acpiphp is used, that surprise  insert/removal is working.
> 
> some laptop like thinkpad, just don't give osc to kernel..
> [    0.505117]  pci0000:00: Requesting ACPI _OSC control (0x1d)
> [    0.505413]  pci0000:00: ACPI _OSC request failed (AE_SUPPORT),
> returned control mask: 0x0d
> [    0.505517] ACPI _OSC control for PCIe not granted, disabling ASPM
> 
> and other laptop give that to kernel, in recent kernel will not give
> acpiphp to have that slot, because it want to hold that for pciehp.
> poor user have to pass 'pci_aspm=off" to disable _OSC for all.
> --- please check the mail that i forward to you yesterday.

Yes, this is a bug, but I'm not sure how to fix it yet.

> Anyway, we do need to let the user to have choice to use acpiphp and pciehp.
> and it should be first come and first serve policy.

And that's why you think they should be modules?  I disagree if so.

Thanks,
Rafael

On Thu, Jan 10, 2013 at 3:39 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, January 10, 2013 03:03:53 PM Yinghai Lu wrote:
>> On Thu, Jan 10, 2013 at 1:50 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > Well, I don't see what functional problems that can bring.
>> >
>> > In theory people may want to have them as modules to avoid loading them on
>> > systems that don't use PCI hotplug, but honestly I think that the complexity
>> > this causes us to deal with is not worth it.
>> >
>> > Moreover, removing the modularity may actually allow us to solve some ordering
>> > issues once and for good.
>>
>> No, the world is not really ideal yet.
>>
>> looks like laptops have problem with pci express cards.
>>
>> when pciehp is used, surprise insert/removal does not work because
>> PresDect does not change properly, so no interrupt is generated.
>> --- i suspects that is silicon problem.
>>
>> but when acpiphp is used, that surprise  insert/removal is working.
>>
>> some laptop like thinkpad, just don't give osc to kernel..
>> [    0.505117]  pci0000:00: Requesting ACPI _OSC control (0x1d)
>> [    0.505413]  pci0000:00: ACPI _OSC request failed (AE_SUPPORT),
>> returned control mask: 0x0d
>> [    0.505517] ACPI _OSC control for PCIe not granted, disabling ASPM
>>
>> and other laptop give that to kernel, in recent kernel will not give
>> acpiphp to have that slot, because it want to hold that for pciehp.
>> poor user have to pass 'pci_aspm=off" to disable _OSC for all.
>> --- please check the mail that i forward to you yesterday.
>
> Yes, this is a bug, but I'm not sure how to fix it yet.

add one command line to control it so do not claim that in osc_control?

>
>> Anyway, we do need to let the user to have choice to use acpiphp and pciehp.
>> and it should be first come and first serve policy.
>
> And that's why you think they should be modules?  I disagree if so.

Yes.

maybe we can have pci=nopciehp in command line just we pci=noaer...

that should handle some corner cases.
--
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 Thursday, January 10, 2013 03:40:45 PM Yinghai Lu wrote:
> On Thu, Jan 10, 2013 at 3:39 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday, January 10, 2013 03:03:53 PM Yinghai Lu wrote:
> >> On Thu, Jan 10, 2013 at 1:50 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> > Well, I don't see what functional problems that can bring.
> >> >
> >> > In theory people may want to have them as modules to avoid loading them on
> >> > systems that don't use PCI hotplug, but honestly I think that the complexity
> >> > this causes us to deal with is not worth it.
> >> >
> >> > Moreover, removing the modularity may actually allow us to solve some ordering
> >> > issues once and for good.
> >>
> >> No, the world is not really ideal yet.
> >>
> >> looks like laptops have problem with pci express cards.
> >>
> >> when pciehp is used, surprise insert/removal does not work because
> >> PresDect does not change properly, so no interrupt is generated.
> >> --- i suspects that is silicon problem.
> >>
> >> but when acpiphp is used, that surprise  insert/removal is working.
> >>
> >> some laptop like thinkpad, just don't give osc to kernel..
> >> [    0.505117]  pci0000:00: Requesting ACPI _OSC control (0x1d)
> >> [    0.505413]  pci0000:00: ACPI _OSC request failed (AE_SUPPORT),
> >> returned control mask: 0x0d
> >> [    0.505517] ACPI _OSC control for PCIe not granted, disabling ASPM
> >>
> >> and other laptop give that to kernel, in recent kernel will not give
> >> acpiphp to have that slot, because it want to hold that for pciehp.
> >> poor user have to pass 'pci_aspm=off" to disable _OSC for all.
> >> --- please check the mail that i forward to you yesterday.
> >
> > Yes, this is a bug, but I'm not sure how to fix it yet.
> 
> add one command line to control it so do not claim that in osc_control?

That's one option, although not very attractive so to speak.

> >> Anyway, we do need to let the user to have choice to use acpiphp and pciehp.
> >> and it should be first come and first serve policy.
> >
> > And that's why you think they should be modules?  I disagree if so.
> 
> Yes.
> 
> maybe we can have pci=nopciehp in command line just we pci=noaer...
>
> that should handle some corner cases.

Yes.  In any case the user should be able to say "I know better", but having
to express that through the "right" ordering of modules is somewhat less than
straightforward in my opinion.

I need to reconsider that code again, maybe I'll figure out what can be done.
It looks like the assumptions it was written with don't really reflect the
reality entirely.

Thanks,
Rafael

Hi,
  I just hit this thread in my bloated Inbox.

Rafael J. Wysocki wrote:
> On Thursday, January 10, 2013 03:03:53 PM Yinghai Lu wrote:
>> On Thu, Jan 10, 2013 at 1:50 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>> Well, I don't see what functional problems that can bring.
>>>
>>> In theory people may want to have them as modules to avoid loading them on
>>> systems that don't use PCI hotplug, but honestly I think that the complexity
>>> this causes us to deal with is not worth it.
>>>
>>> Moreover, removing the modularity may actually allow us to solve some ordering
>>> issues once and for good.
>>
>> No, the world is not really ideal yet.
>>
>> looks like laptops have problem with pci express cards.
>>
>> when pciehp is used, surprise insert/removal does not work because
>> PresDect does not change properly, so no interrupt is generated.
>> --- i suspects that is silicon problem.

That's what seemed to be the conclusion half a year ago around 3.2.x/3.3.x
for my issues as well (SandyBridge C6/C200 chipset).

>>
>> but when acpiphp is used, that surprise  insert/removal is working.

That's what I discovered few days ago as well. However, there are still some
differences between individual express cards and I just need to find some time to
dig through the data I collected.

>>
>> some laptop like thinkpad, just don't give osc to kernel..
>> [    0.505117]  pci0000:00: Requesting ACPI _OSC control (0x1d)
>> [    0.505413]  pci0000:00: ACPI _OSC request failed (AE_SUPPORT),
>> returned control mask: 0x0d
>> [    0.505517] ACPI _OSC control for PCIe not granted, disabling ASPM
>>
>> and other laptop give that to kernel, in recent kernel will not give
>> acpiphp to have that slot, because it want to hold that for pciehp.
>> poor user have to pass 'pci_aspm=off" to disable _OSC for all.
>> --- please check the mail that i forward to you yesterday.
> 
> Yes, this is a bug, but I'm not sure how to fix it yet.

Looks like what I see with Dell Vostro 3550 as well.

> 
>> Anyway, we do need to let the user to have choice to use acpiphp and pciehp.
>> and it should be first come and first serve policy.
> 
> And that's why you think they should be modules?  I disagree if so.

For me it is easier to cold boot with a card plugged in and fiddle later with
hotplug if I want to unload the card. Until that, I can inspect wheteher PresDet
really reports the card is in, and the see if system reports same after loading
acpiphp or pciehp. I wouldn't drop the possibility to have them as modules, at
least for now when finally we have some clue what is going on and can load the
modules as we want while chasing the bugs.

But sorry for hijacking this thread, maybe I managed to delete your answers on my
thread ("Re: Dell Vostro 3550: pci_hotplug+acpiphp require 'pcie_aspm=force' on kernel command-line for hotplug to work").
Will go through web archives to make sure I did not miss something.

Martin
--
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, January 11, 2013 12:07:43 PM Martin Mokrejs wrote:
> Hi,
>   I just hit this thread in my bloated Inbox.
> 
> Rafael J. Wysocki wrote:
> > On Thursday, January 10, 2013 03:03:53 PM Yinghai Lu wrote:
> >> On Thu, Jan 10, 2013 at 1:50 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >>> Well, I don't see what functional problems that can bring.
> >>>
> >>> In theory people may want to have them as modules to avoid loading them on
> >>> systems that don't use PCI hotplug, but honestly I think that the complexity
> >>> this causes us to deal with is not worth it.
> >>>
> >>> Moreover, removing the modularity may actually allow us to solve some ordering
> >>> issues once and for good.
> >>
> >> No, the world is not really ideal yet.
> >>
> >> looks like laptops have problem with pci express cards.
> >>
> >> when pciehp is used, surprise insert/removal does not work because
> >> PresDect does not change properly, so no interrupt is generated.
> >> --- i suspects that is silicon problem.
> 
> That's what seemed to be the conclusion half a year ago around 3.2.x/3.3.x
> for my issues as well (SandyBridge C6/C200 chipset).
> 
> >>
> >> but when acpiphp is used, that surprise  insert/removal is working.
> 
> That's what I discovered few days ago as well. However, there are still some
> differences between individual express cards and I just need to find some time to
> dig through the data I collected.
> 
> >>
> >> some laptop like thinkpad, just don't give osc to kernel..
> >> [    0.505117]  pci0000:00: Requesting ACPI _OSC control (0x1d)
> >> [    0.505413]  pci0000:00: ACPI _OSC request failed (AE_SUPPORT),
> >> returned control mask: 0x0d
> >> [    0.505517] ACPI _OSC control for PCIe not granted, disabling ASPM
> >>
> >> and other laptop give that to kernel, in recent kernel will not give
> >> acpiphp to have that slot, because it want to hold that for pciehp.
> >> poor user have to pass 'pci_aspm=off" to disable _OSC for all.
> >> --- please check the mail that i forward to you yesterday.
> > 
> > Yes, this is a bug, but I'm not sure how to fix it yet.
> 
> Looks like what I see with Dell Vostro 3550 as well.
> 
> > 
> >> Anyway, we do need to let the user to have choice to use acpiphp and pciehp.
> >> and it should be first come and first serve policy.
> > 
> > And that's why you think they should be modules?  I disagree if so.
> 
> For me it is easier to cold boot with a card plugged in and fiddle later with
> hotplug if I want to unload the card. Until that, I can inspect wheteher PresDet
> really reports the card is in, and the see if system reports same after loading
> acpiphp or pciehp. I wouldn't drop the possibility to have them as modules, at
> least for now when finally we have some clue what is going on and can load the
> modules as we want while chasing the bugs.

The problem with modules is that the initialization ordering depends on the
order in which the modules are loaded.  If there are two modules, there are 2
different ways, if there are 3 modules, there are 6 ways and so on.  You get
the idea. :-)

Now, we need to take all of the possible orderings in the code and that's
quite complicated.  Also it is easy to leave one ordering untested if the
distro you use for testing happens to prefer a different one.  So the fact that
they are modules very well may be the _source_ of the problems you're seeing.

Thanks,
Rafael

On Thu, Jan 10, 2013 at 4:59 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, January 10, 2013 03:40:45 PM Yinghai Lu wrote:
>> On Thu, Jan 10, 2013 at 3:39 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > On Thursday, January 10, 2013 03:03:53 PM Yinghai Lu wrote:
>> >> On Thu, Jan 10, 2013 at 1:50 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> >> > Well, I don't see what functional problems that can bring.
>> >> >
>> >> > In theory people may want to have them as modules to avoid loading them on
>> >> > systems that don't use PCI hotplug, but honestly I think that the complexity
>> >> > this causes us to deal with is not worth it.
>> >> >
>> >> > Moreover, removing the modularity may actually allow us to solve some ordering
>> >> > issues once and for good.
>> >>
>> >> No, the world is not really ideal yet.
>> >>
>> >> looks like laptops have problem with pci express cards.
>> >>
>> >> when pciehp is used, surprise insert/removal does not work because
>> >> PresDect does not change properly, so no interrupt is generated.
>> >> --- i suspects that is silicon problem.
>> >>
>> >> but when acpiphp is used, that surprise  insert/removal is working.
>> >>
>> >> some laptop like thinkpad, just don't give osc to kernel..
>> >> [    0.505117]  pci0000:00: Requesting ACPI _OSC control (0x1d)
>> >> [    0.505413]  pci0000:00: ACPI _OSC request failed (AE_SUPPORT),
>> >> returned control mask: 0x0d
>> >> [    0.505517] ACPI _OSC control for PCIe not granted, disabling ASPM
>> >>
>> >> and other laptop give that to kernel, in recent kernel will not give
>> >> acpiphp to have that slot, because it want to hold that for pciehp.
>> >> poor user have to pass 'pci_aspm=off" to disable _OSC for all.
>> >> --- please check the mail that i forward to you yesterday.
>> >
>> > Yes, this is a bug, but I'm not sure how to fix it yet.
>>
>> add one command line to control it so do not claim that in osc_control?
>
> That's one option, although not very attractive so to speak.
>
>> >> Anyway, we do need to let the user to have choice to use acpiphp and pciehp.
>> >> and it should be first come and first serve policy.
>> >
>> > And that's why you think they should be modules?  I disagree if so.
>>
>> Yes.
>>
>> maybe we can have pci=nopciehp in command line just we pci=noaer...
>>
>> that should handle some corner cases.
>
> Yes.  In any case the user should be able to say "I know better", but having
> to express that through the "right" ordering of modules is somewhat less than
> straightforward in my opinion.

I think it's fine if a user *can* override the default ordering.

I am completely opposed to *requiring* a user to supply a command line
option or change the order of module loading just to get hotplug to
work.  There's no sane way to document that or communicate that
information to users.

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, January 11, 2013 11:06:29 AM Bjorn Helgaas wrote:
> On Thu, Jan 10, 2013 at 4:59 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday, January 10, 2013 03:40:45 PM Yinghai Lu wrote:
> >> On Thu, Jan 10, 2013 at 3:39 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> > On Thursday, January 10, 2013 03:03:53 PM Yinghai Lu wrote:
> >> >> On Thu, Jan 10, 2013 at 1:50 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> >> > Well, I don't see what functional problems that can bring.
> >> >> >
> >> >> > In theory people may want to have them as modules to avoid loading them on
> >> >> > systems that don't use PCI hotplug, but honestly I think that the complexity
> >> >> > this causes us to deal with is not worth it.
> >> >> >
> >> >> > Moreover, removing the modularity may actually allow us to solve some ordering
> >> >> > issues once and for good.
> >> >>
> >> >> No, the world is not really ideal yet.
> >> >>
> >> >> looks like laptops have problem with pci express cards.
> >> >>
> >> >> when pciehp is used, surprise insert/removal does not work because
> >> >> PresDect does not change properly, so no interrupt is generated.
> >> >> --- i suspects that is silicon problem.
> >> >>
> >> >> but when acpiphp is used, that surprise  insert/removal is working.
> >> >>
> >> >> some laptop like thinkpad, just don't give osc to kernel..
> >> >> [    0.505117]  pci0000:00: Requesting ACPI _OSC control (0x1d)
> >> >> [    0.505413]  pci0000:00: ACPI _OSC request failed (AE_SUPPORT),
> >> >> returned control mask: 0x0d
> >> >> [    0.505517] ACPI _OSC control for PCIe not granted, disabling ASPM
> >> >>
> >> >> and other laptop give that to kernel, in recent kernel will not give
> >> >> acpiphp to have that slot, because it want to hold that for pciehp.
> >> >> poor user have to pass 'pci_aspm=off" to disable _OSC for all.
> >> >> --- please check the mail that i forward to you yesterday.
> >> >
> >> > Yes, this is a bug, but I'm not sure how to fix it yet.
> >>
> >> add one command line to control it so do not claim that in osc_control?
> >
> > That's one option, although not very attractive so to speak.
> >
> >> >> Anyway, we do need to let the user to have choice to use acpiphp and pciehp.
> >> >> and it should be first come and first serve policy.
> >> >
> >> > And that's why you think they should be modules?  I disagree if so.
> >>
> >> Yes.
> >>
> >> maybe we can have pci=nopciehp in command line just we pci=noaer...
> >>
> >> that should handle some corner cases.
> >
> > Yes.  In any case the user should be able to say "I know better", but having
> > to express that through the "right" ordering of modules is somewhat less than
> > straightforward in my opinion.
> 
> I think it's fine if a user *can* override the default ordering.
> 
> I am completely opposed to *requiring* a user to supply a command line
> option or change the order of module loading just to get hotplug to
> work.  There's no sane way to document that or communicate that
> information to users.

Well, I agree that neither of these things should be necessary, but that
means the current code is based on incorrect assumptions.

Perhaps our mistake is to believe that once we've done the _OSC handshake,
we are supposed not to use ACPI for hotplug events handling?  It looks like
the general expectation of BIOS writers is that we will use ACPI regardless
if available and we *may* use the native PCIe signaling in addition to it if
the _OSC handshake is successful.

So maybe there should be just one PCI hotplug driver that can use both
ACPI and PCIe native events?  [The same applies to PM, but the situation is
a bit simpler in there.]

Thanks,
Rafael