| Message ID | 20230719163446.1398961-4-samuel.holland@sifive.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpio: sifive: Module support | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Series has a cover letter |
| conchuod/tree_selection | success | Guessed tree name to be for-next at HEAD 471aba2e4760 |
| conchuod/fixes_present | success | Fixes tag not required for -next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 4 and now 4 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 9 this patch: 9 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 9 this patch: 9 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 3 this patch: 3 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 52 lines checked |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
On Wed, Jul 19, 2023 at 09:34:44AM -0700, Samuel Holland wrote: > Do not parse the devicetree again when the data is already available > from the IRQ subsystem. This follows the example of the ThunderX and > X-Gene GPIO drivers. The ngpio check is needed to avoid a possible > out-of-bounds read. ... > - girq->parent_domain = parent; > + girq->parent_domain = irq_get_irq_data(chip->irq_number[0])->domain; For the sake of readability I would like to leave parent variable and assign it beforehand somewhere upper in the code. Also, can irq_get_irq_data() return NULL? Needs a comment on top of that assignment or an additional check.
On 2023-07-19 11:54 AM, Andy Shevchenko wrote: > On Wed, Jul 19, 2023 at 09:34:44AM -0700, Samuel Holland wrote: >> Do not parse the devicetree again when the data is already available >> from the IRQ subsystem. This follows the example of the ThunderX and >> X-Gene GPIO drivers. The ngpio check is needed to avoid a possible >> out-of-bounds read. > > ... > >> - girq->parent_domain = parent; >> + girq->parent_domain = irq_get_irq_data(chip->irq_number[0])->domain; > > For the sake of readability I would like to leave parent variable > and assign it beforehand somewhere upper in the code. OK. > Also, can irq_get_irq_data() return NULL? Needs a comment on top > of that assignment or an additional check. No, the earlier loop already verified the IRQ number was valid. I don't think it can later become invalid. In any case, we already dereference the result of irq_get_irq_data(irq_number[foo]) in sifive_gpio_child_to_parent_hwirq().
On Wed, Jul 19, 2023 at 12:03:46PM -0500, Samuel Holland wrote: > On 2023-07-19 11:54 AM, Andy Shevchenko wrote: > > On Wed, Jul 19, 2023 at 09:34:44AM -0700, Samuel Holland wrote: ... > > Also, can irq_get_irq_data() return NULL? Needs a comment on top > > of that assignment or an additional check. > > No, the earlier loop already verified the IRQ number was valid. I don't think it > can later become invalid. In any case, we already dereference the result of > irq_get_irq_data(irq_number[foo]) in sifive_gpio_child_to_parent_hwirq(). Thanks for explanation, just add a comment.
diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c index 6606c919d957..46a42109d6f5 100644 --- a/drivers/gpio/gpio-sifive.c +++ b/drivers/gpio/gpio-sifive.c @@ -6,7 +6,6 @@ #include <linux/bitops.h> #include <linux/device.h> #include <linux/errno.h> -#include <linux/of_irq.h> #include <linux/gpio/driver.h> #include <linux/init.h> #include <linux/platform_device.h> @@ -180,9 +179,6 @@ static const struct regmap_config sifive_gpio_regmap_config = { static int sifive_gpio_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; - struct device_node *node = pdev->dev.of_node; - struct device_node *irq_parent; - struct irq_domain *parent; struct gpio_irq_chip *girq; struct sifive_gpio *chip; int ret, ngpio; @@ -202,24 +198,16 @@ static int sifive_gpio_probe(struct platform_device *pdev) if (IS_ERR(chip->regs)) return PTR_ERR(chip->regs); - irq_parent = of_irq_find_parent(node); - if (!irq_parent) { - dev_err(dev, "no IRQ parent node\n"); - return -ENODEV; - } - parent = irq_find_host(irq_parent); - of_node_put(irq_parent); - if (!parent) { - dev_err(dev, "no IRQ parent domain\n"); - return -ENODEV; - } - for (ngpio = 0; ngpio < SIFIVE_GPIO_MAX; ngpio++) { ret = platform_get_irq_optional(pdev, ngpio); if (ret < 0) break; chip->irq_number[ngpio] = ret; } + if (!ngpio) { + dev_err(dev, "no IRQ found\n"); + return -ENODEV; + } ret = bgpio_init(&chip->gc, dev, 4, chip->base + SIFIVE_GPIO_INPUT_VAL, @@ -248,7 +236,7 @@ static int sifive_gpio_probe(struct platform_device *pdev) girq = &chip->gc.irq; gpio_irq_chip_set_chip(girq, &sifive_gpio_irqchip); girq->fwnode = dev->fwnode; - girq->parent_domain = parent; + girq->parent_domain = irq_get_irq_data(chip->irq_number[0])->domain; girq->child_to_parent_hwirq = sifive_gpio_child_to_parent_hwirq; girq->handler = handle_bad_irq; girq->default_type = IRQ_TYPE_NONE;
Do not parse the devicetree again when the data is already available from the IRQ subsystem. This follows the example of the ThunderX and X-Gene GPIO drivers. The ngpio check is needed to avoid a possible out-of-bounds read. Signed-off-by: Samuel Holland <samuel.holland@sifive.com> --- Changes in v2: - New patch for v2 drivers/gpio/gpio-sifive.c | 22 +++++----------------- 1 file changed, 5 insertions(+), 17 deletions(-)