Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754659AbbIABrK (ORCPT ); Mon, 31 Aug 2015 21:47:10 -0400 Received: from szxga01-in.huawei.com ([58.251.152.64]:2314 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754466AbbIABrI (ORCPT ); Mon, 31 Aug 2015 21:47:08 -0400 Subject: Re: [PATCH v4 1/2] Add the driver of mbigen interrupt controller To: Alexey Klimov References: CC: Catalin Marinas , Linux Kernel Mailing List , , Will Deacon , Mark Rutland , , , Thomas Gleixner , , , , , Kenneth Lee , , , , , , , Zhangfei Gao , From: "majun (F)" Message-ID: <55E50344.9090104@huawei.com> Date: Tue, 1 Sep 2015 09:45:40 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.177.235.245] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6985 Lines: 229 Hi Alexey: 在 2015/8/29 11:13, Alexey Klimov 写道: > Hi Ma Jun, > > On Wed, Aug 19, 2015 at 5:55 AM, MaJun wrote: >> From: Ma Jun >> >> Mbigen means Message Based Interrupt Generator(MBIGEN). >> >> Its a kind of interrupt controller that collects >> >> the interrupts from external devices and generate msi interrupt. >> >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include > > What do you think about sorting this? ok > > >> +#include "irqchip.h" >> + [...] >> +*/ >> +static u32 calc_irq_index(struct mbigen_device *dev, u32 nid, u32 offset) >> +{ >> + struct mbigen_node *mgn_node = NULL, *tmp; >> + unsigned long flags; >> + u32 index = 0; >> + >> + raw_spin_lock_irqsave(&dev->lock, flags); >> + list_for_each_entry(tmp, &dev->mbigen_node_list, entry) { >> + if (tmp->node_num == nid) >> + mgn_node = tmp; >> + } >> + raw_spin_unlock_irqrestore(&dev->lock, flags); >> + >> + if (mgn_node == NULL) { >> + pr_err("No mbigen node found in device:%s\n", >> + dev->node->full_name); >> + return -ENXIO; >> + } >> + >> + if ((offset <= (mgn_node->pin_offset + mgn_node->irq_nr)) >> + && (offset >= mgn_node->pin_offset)) >> + index = mgn_node->index_offset + (offset - mgn_node->pin_offset); >> + else { >> + pr_err("Err: no invalid index\n"); > > Please check this message. > 1. I don't know all details about this driver but is it really correct > "no invalid index"? Maybe you mean "no vaild index" or just "invalid > index"? > Just checking if i correctly understand this. > You are right. This should be "no valid index" > 2. Imagine what info user/dmesg reader gets when she or he will see > such message? I suggest to add some info about driver that printed > this message. > You already have nice name in mbigen_irq_chip: "MBIGEN-v2". What do > you think about using it as prefix in your printk-based messages? > Please also consider revisiting other messages in this patch. > good suggestion. > >> + index = -EINVAL; >> + } >> + >> + return index; >> +} [...] >> + INIT_LIST_HEAD(dev_to_msi_list(&mgn_dev->dev)); >> + >> + ret = platform_msi_domain_alloc_irqs(&mgn_dev->dev, >> + nvec, mbigen_write_msg); >> + if (ret) >> + goto out_free_dev; >> + >> + INIT_LIST_HEAD(&mgn_dev->entry); >> + raw_spin_lock_init(&mgn_dev->lock); >> + INIT_LIST_HEAD(&mgn_dev->mbigen_node_list); >> + >> + /* Parse and get the info of mbigen nodes this >> + * device connected >> + */ >> + parse_mbigen_node(mgn_dev); >> + >> + mgn_irq_data = kcalloc(nvec, sizeof(*mgn_irq_data), GFP_KERNEL); >> + if (!mgn_irq_data) >> + return -ENOMEM; > > Hm. Do you need error path here instead of simple return -ENOMEM? > Maybe 'goto out_free_dev' will work for you. Right. Memory leak happened. > >> + mgn_dev->mgn_data = mgn_irq_data; >> + >> + for_each_msi_entry(desc, &mgn_dev->dev) { >> + mbigen_set_irq_handler_data(desc, mgn_dev, >> + &mgn_irq_data[desc->platform.msi_index]); >> + irq_set_chained_handler(desc->irq, mbigen_handle_cascade_irq); >> + } >> + >> + raw_spin_lock(&chip->lock); >> + list_add(&mgn_dev->entry, &chip->mbigen_device_list); >> + raw_spin_unlock(&chip->lock); >> + >> + return 0; >> + >> +out_free_dev: >> + kfree(mgn_dev); >> + pr_err("failed to get MSIs for device:%s (%d)\n", node->full_name, >> + ret); >> + return ret; >> +} >> + >> +/* >> + * Early initialization as an interrupt controller >> + */ >> +static int __init mbigen_of_init(struct device_node *node) >> +{ >> + struct mbigen_chip *mgn_chip; >> + struct device_node *child; >> + struct irq_domain *domain; >> + void __iomem *base; >> + int err; >> + >> + base = of_iomap(node, 0); >> + if (!base) { >> + pr_err("%s: unable to map registers\n", node->full_name); >> + return -ENOMEM; >> + } >> + >> + mgn_chip = kzalloc(sizeof(*mgn_chip), GFP_KERNEL); >> + if (!mgn_chip) { >> + err = -ENOMEM; >> + goto unmap_reg; >> + } >> + >> + mgn_chip->base = base; >> + mgn_chip->node = node; >> + >> + domain = irq_domain_add_tree(node, &mbigen_domain_ops, mgn_chip); >> + mgn_chip->domain = domain; >> + >> + raw_spin_lock_init(&mgn_chip->lock); >> + INIT_LIST_HEAD(&mgn_chip->entry); >> + INIT_LIST_HEAD(&mgn_chip->mbigen_device_list); >> + >> + for_each_child_of_node(node, child) { >> + mbigen_device_init(mgn_chip, child); > > You don't check error from mbigen_device_init() I don't think we need to check errors here. mbigen_device_init() handle all errors. Thanks Ma Jun > >> + } >> + >> + spin_lock(&mbigen_chip_lock); >> + list_add(&mgn_chip->entry, &mbigen_chip_list); >> + spin_unlock(&mbigen_chip_lock); >> + >> + return 0; >> + >> +unmap_reg: >> + iounmap(base); >> + pr_err("MBIGEN: failed probing:%s (%d)\n", node->full_name, err); >> + return err; >> +} >> + >> +static struct of_device_id mbigen_chip_id[] = { >> + { .compatible = "hisilicon,mbigen-v2",}, >> + {}, >> +}; >> + >> +static int __init mbigen_init(void) >> +{ >> + struct device_node *np; >> + >> + for (np = of_find_matching_node(NULL, mbigen_chip_id); np; >> + np = of_find_matching_node(np, mbigen_chip_id)) { >> + mbigen_of_init(np); >> + } >> + >> + return 0; >> +} >> + >> +core_initcall(mbigen_init); >> + >> +MODULE_AUTHOR("Jun Ma "); >> +MODULE_AUTHOR("Yun Wu "); >> +MODULE_LICENSE("GPL"); >> +MODULE_DESCRIPTION("Hisilicon MBI Generator driver"); >> -- >> 1.7.1 >> >> >> -- >> 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/ > > > -- 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/