linux-kernel - Re: [PATCH v8 09/22] PCI, ACPI: Add pci_root

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAE9FiQUdXWxrM9Mk4rPG4fb5DKEP46Oa84219y8KC+kOaLB67w@mail.gmail.com>
Date:	Mon, 14 Jan 2013 22:45:02 -0800
From:	Yinghai Lu <yinghai@...nel.org>
To:	"Rafael J. Wysocki" <rjw@...k.pl>
Cc:	Bjorn Helgaas <bhelgaas@...gle.com>, Len Brown <lenb@...nel.org>,
	Taku Izumi <izumi.taku@...fujitsu.com>,
	Jiang Liu <jiang.liu@...wei.com>, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org
Subject: Re: [PATCH v8 09/22] PCI, ACPI: Add pci_root_hp hot removal
 notification support.

On Sat, Jan 12, 2013 at 3:26 PM, Rafael J. Wysocki <rjw@...k.pl> wrote:
> On Friday, January 11, 2013 02:40:36 PM Yinghai Lu wrote:
>> Add missing hot_remove support for root device.
>>
>> How to test it?
>> Find out root bus number to acpi root name mapping from dmesg or /sys
>>
>>   echo "\_SB.PCIB 3" > /sys/kernel/debug/acpi/sci_notify
>> to remove root bus
>>
>> -v2: separate stop and remove, so it will be safe for comingi
>>       acpi_pci_bind_notify() changes.
>>
>> Signed-off-by: Yinghai Lu <yinghai@...nel.org>
>> Cc: Len Brown <lenb@...nel.org>
>> Cc: linux-acpi@...r.kernel.org
>> ---
>>  drivers/acpi/pci_root.c |   67 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 67 insertions(+)
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index 5c1f462c..5ae36d8 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -740,6 +740,12 @@ static void add_acpi_root_bridge(acpi_handle handle)
>>       list_add(&bridge->list, &acpi_root_bridge_list);
>>  }
>>
>> +static void remove_acpi_root_bridge(struct acpi_root_bridge *bridge)
>> +{
>> +     list_del(&bridge->list);
>> +     kfree(bridge);
>> +}
>> +
>>  struct acpi_root_hp_work {
>>       struct work_struct work;
>>       acpi_handle handle;
>> @@ -800,6 +806,61 @@ static void handle_root_bridge_insertion(acpi_handle handle)
>>               printk(KERN_ERR "cannot add bridge to acpi list\n");
>>  }
>>
>> +static int acpi_root_evaluate_object(acpi_handle handle, char *cmd, int val)
>> +{
>> +     acpi_status status;
>> +     struct acpi_object_list arg_list;
>> +     union acpi_object arg;
>> +
>> +     arg_list.count = 1;
>> +     arg_list.pointer = &arg;
>> +     arg.type = ACPI_TYPE_INTEGER;
>> +     arg.integer.value = val;
>> +
>> +     status = acpi_evaluate_object(handle, cmd, &arg_list, NULL);
>> +     if (ACPI_FAILURE(status)) {
>> +             pr_warn("%s: %s to %d failed\n",
>> +                              __func__, cmd, val);
>> +             return -1;
>
> Please use a meaningful error code.
>
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static void handle_root_bridge_removal(acpi_handle handle,
>> +              struct acpi_root_bridge *bridge)
>> +{
>> +     u32 flags = 0;
>> +     struct acpi_device *device;
>> +
>> +     if (bridge) {
>> +             flags = bridge->flags;
>> +             remove_acpi_root_bridge(bridge);
>> +     }
>> +
>> +     if (!acpi_bus_get_device(handle, &device)) {
>> +             int ret_val;
>> +
>> +             /* remove pci devices at first */
>> +             ret_val = acpi_bus_trim(device, 0);
>> +             printk(KERN_DEBUG "acpi_bus_trim stop return %x\n", ret_val);
>> +
>> +             /* remove acpi devices */
>> +             ret_val = acpi_bus_trim(device, 1);
>> +             printk(KERN_DEBUG "acpi_bus_trim remove return %x\n", ret_val);
>
> First of all, I don't agree with the way acpi_bus_trim() is used here, as
> I said in the previous message.
>
> Second, this code duplicates the code you're adding on [08/22] almost exactly.
> Please put it into a one separate function instead of duplicating it like this.
>
>> +     }
>> +
>> +     if (flags & ROOT_BRIDGE_HAS_PS3) {
>> +             acpi_status status;
>> +
>> +             status = acpi_evaluate_object(handle, "_PS3", NULL, NULL);
>> +             if (ACPI_FAILURE(status))
>> +                     pr_warn("%s: _PS3 failed\n", __func__);
>
> No, please.  acpi_device_set_power() is for that.
>
>> +     }
>> +     if (flags & ROOT_BRIDGE_HAS_EJ0)
>> +             acpi_root_evaluate_object(handle, "_EJ0", 1);
>
> That seems to duplicate some code from scan.c.

yes, will address in the following patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/