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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 6 Sep 2016 13:29:09 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Tomasz Nowicki <tn@...ihalf.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <marc.zyngier@....com>,
        Jason Cooper <jason@...edaemon.net>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Bjorn Helgaas <helgaas@...nel.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Lorenzo Pieralisi <Lorenzo.Pieralisi@....com>,
        Will Deacon <will.deacon@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        Hanjun Guo <hanjun.guo@...aro.org>, shijie.huang@....com,
        robert.richter@...iumnetworks.com, Marcin Wojtas <mw@...ihalf.com>,
        Linux PCI <linux-pci@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linaro-acpi@...ts.linaro.org" <linaro-acpi@...ts.linaro.org>,
        Andrea Gallo <andrea.gallo@...aro.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Al Stone <al.stone@...aro.org>,
        G Gregory <graeme.gregory@...aro.org>,
        David Daney <ddaney.cavm@...il.com>,
        Sinan Kaya <okaya@...eaurora.org>
Subject: Re: [PATCH V10 7/8] irqchip/gicv3-its: Factor out PCI-MSI part that
 might be reused for ACPI

On Tue, Sep 6, 2016 at 12:22 PM, Tomasz Nowicki <tn@...ihalf.com> wrote:
> Hi Thomas,
>
>
> On 06.09.2016 11:50, Thomas Gleixner wrote:
>>
>> On Tue, 6 Sep 2016, Tomasz Nowicki wrote:
>>>
>>> -static int __init its_pci_msi_init(void)
>>> +static int __init its_pci_msi_init_one(struct fwnode_handle *handle,
>>> +                                      const char *name)
>>>  {
>>> -       struct device_node *np;
>>>         struct irq_domain *parent;
>>>
>>> +       parent = irq_find_matching_fwnode(handle, DOMAIN_BUS_NEXUS);
>>> +       if (!parent || !msi_get_domain_info(parent)) {
>>> +               pr_err("%s: Unable to locate ITS domain\n", name);
>>> +               return -ENXIO;
>>> +       }
>>> +
>>> +       if (!pci_msi_create_irq_domain(handle, &its_pci_msi_domain_info,
>>> +                                      parent)) {
>>> +               pr_err("%s: Unable to create PCI domain\n", name);
>>> +               return -ENOMEM;
>>
>>
>> So you have error codes here.
>>
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int __init its_pci_of_msi_init(void)
>>> +{
>>> +       struct device_node *np;
>>> +
>>>         for (np = of_find_matching_node(NULL, its_device_id); np;
>>>              np = of_find_matching_node(np, its_device_id)) {
>>>                 if (!of_property_read_bool(np, "msi-controller"))
>>>                         continue;
>>>
>>> -               parent = irq_find_matching_host(np, DOMAIN_BUS_NEXUS);
>>> -               if (!parent || !msi_get_domain_info(parent)) {
>>> -                       pr_err("%s: unable to locate ITS domain\n",
>>> -                              np->full_name);
>>> +               if (its_pci_msi_init_one(of_node_to_fwnode(np),
>>> np->full_name))
>>>                         continue;
>>> -               }
>>> -
>>> -               if (!pci_msi_create_irq_domain(of_node_to_fwnode(np),
>>> -                                              &its_pci_msi_domain_info,
>>> -                                              parent)) {
>>> -                       pr_err("%s: unable to create PCI domain\n",
>>> -                              np->full_name);
>>> -                       continue;
>>> -               }
>>>
>>>                 pr_info("PCI/MSI: %s domain created\n", np->full_name);
>>>         }
>>>
>>>         return 0;
>>
>>
>> And no matter what you return success
>>
>>>  }
>>> +
>>> +static int __init its_pci_msi_init(void)
>>> +{
>>> +       its_pci_of_msi_init();
>>
>>
>> Which you subsequently ignore here. What's the point?
>>
>
> Well, this patch is meant to refactor the code. It does not change the error
> handing nor the functionality. However, you have got a point but IMO it
> should be fixed in separate patch if possible.

But you are refactoring the code, so it would make sense to clean it
up while at it.  Perhaps add one more patch to the series?

Thanks,
Rafael

Powered by blists - more mailing lists