| 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
| ||
|
Message-ID: <57078BE8.8040604@arm.com>
Date: Fri, 8 Apr 2016 11:46:00 +0100
From: Marc Zyngier <marc.zyngier@....com>
To: Kefeng Wang <wangkefeng.wang@...wei.com>
Cc: Jason Cooper <jason@...edaemon.net>, majun258@...wei.com,
guohanjun@...wei.com, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] irqchip/mbigen: Display message of MBIGEN domain created
On 08/04/16 11:33, Kefeng Wang wrote:
>
>
> On 2016/4/8 16:53, Marc Zyngier wrote:
>> On Fri, 8 Apr 2016 16:26:21 +0800
>> Kefeng Wang <wangkefeng.wang@...wei.com> wrote:
>
>>>> Overall, this doesn't look like a critical patch to me. I think Ma Jun
>>>> is working on separate series reworking the way the mgigen is getting
>>>> probed, so I'd advise you to work with him in order to integrate this
>>>> patch in his series, as it would make a lot more sense.
>>>
>>> When try to enable hip06 d03 board[1], we met following error log, so I add
>>> some debug message. The mbigen driver use module_platform_driver, the driver
>>> initialization is too late, and it is without any message, we don't know
>>> about any info of mbigen. I think we should show something about the mbigen
>>> domain creation at least. What's your option?
>>>
>>> Is there a way to solve this improper print？
>>> -----------
>>> [ 1.345945] irq: no irq domain found for /mbigen_pcie@...80000/intc_usb !
>>> [ 1.353660] irq: no irq domain found for /mbigen_pcie@...80000/intc_usb !
>>
>> How can printing anything solve this issue? Furthermore, the error
>> message you quote is pretty explicit: no mbigen for you, move along.
>>
>> There is a long standing dependency issue for interrupt controllers
>> that are also platform devices, and until you resolve (or help
>> resolving) that issue, you will get that kind of problem. As I
>> mentioned countless times (both on list and in person), you only have
>> two options:
>>
>> - either you defer probing devices behind the mbigen until the mbigen
>> itself is up and running
>> - or you solve the generic dependency problem.
>>
> Ok，got it，thanks for your explanation.
>> I feel a bit like a stuck record here.
>
> But there is still one issue in the for_each_child_of_node loop of
> mbigen_device_probe. Assume that there are 3+ child node(mbigen_gmac,
> mbigen_i2c, mbigen_xxx, etc) in mbigen_chip_dsa, if one of them with
> incorrect configuration, then the loop will end, but we still need
> parse the left child node. we should consider this situation, right?
That's up to whoever designed the driver to decide, really. I don't know
if it is worth continuing in that case (and you are in a better position
than me to find out).
> How about split that part into a new mbigen_create_domain(), shown below, and display
> the result of mbigen domain creation in it.
>
>
> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
> index d67baa2..9c1d22e 100644
> --- a/drivers/irqchip/irq-mbigen.c
> +++ b/drivers/irqchip/irq-mbigen.c
> @@ -236,15 +236,41 @@ static struct irq_domain_ops mbigen_domain_ops = {
> .free = irq_domain_free_irqs_common,
> };
>
> -static int mbigen_device_probe(struct platform_device *pdev)
> +static void mbigen_create_domain(struct mbigen_device *mgn_dev, struct device_node *np)
> {
> - struct mbigen_device *mgn_chip;
> + struct device *parent = platform_bus_type.dev_root;
> + struct device *dev = &mgn_dev->pdev->dev;
> struct platform_device *child;
> struct irq_domain *domain;
> + u32 num_pins;
> +
> + if (!of_property_read_bool(np, "interrupt-controller"))
> + goto err;
> +
> + if (of_property_read_u32(np, "num-pins", &num_pins))
> + goto err;
> +
> + child = of_platform_device_create(np, NULL, parent);
> + if (IS_ERR(child))
> + goto err;
> +
> + domain = platform_msi_create_device_domain(&child->dev, num_pins,
> + mbigen_write_msg,
> + &mbigen_domain_ops,
> + mgn_dev);
> + if (!domain)
> + goto err;
> +
> + dev_info(dev, "%s domain created\n", np->full_name);
You're probably missing a "return" here.
> +err:
> + dev_err(dev, "unable to create %s domain\n", np->full_name);
> +}
> +
> +static int mbigen_device_probe(struct platform_device *pdev)
> +{
> + struct mbigen_device *mgn_chip;
> struct device_node *np;
> - struct device *parent;
> struct resource *res;
> - u32 num_pins;
>
> mgn_chip = devm_kzalloc(&pdev->dev, sizeof(*mgn_chip), GFP_KERNEL);
> if (!mgn_chip)
> @@ -257,28 +283,8 @@ static int mbigen_device_probe(struct platform_device *pdev)
> if (IS_ERR(mgn_chip->base))
> return PTR_ERR(mgn_chip->base);
>
> - for_each_child_of_node(pdev->dev.of_node, np) {
> - if (!of_property_read_bool(np, "interrupt-controller"))
> - continue;
> -
> - parent = platform_bus_type.dev_root;
> - child = of_platform_device_create(np, NULL, parent);
> - if (IS_ERR(child))
> - return PTR_ERR(child);
> -
> - if (of_property_read_u32(child->dev.of_node, "num-pins",
> - &num_pins) < 0) {
> - dev_err(&pdev->dev, "No num-pins property\n");
> - return -EINVAL;
> - }
> -
> - domain = platform_msi_create_device_domain(&child->dev, num_pins,
> - mbigen_write_msg,
> - &mbigen_domain_ops,
> - mgn_chip);
> - if (!domain)
> - return -ENOMEM;
> - }
> + for_each_child_of_node(pdev->dev.of_node, np)
> + mbigen_create_domain(mgn_chip, np);
>
> platform_set_drvdata(pdev, mgn_chip);
In general, I'd suggest you work with Ma Jun, as he maintains that driver.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Powered by blists - more mailing lists