[PATCH v2 0/4] gpio: sifive: Module support

[PATCH v2 0/4] gpio: sifive: Module support

[Samuel Holland]

With the call to of_irq_count() removed, the SiFive GPIO driver can be
built as a module. This helps to minimize the size of a multiplatform
kernel, and is required by some downstream distributions (Android GKI).

This series removes the rest of the of_* API usage in the process.

Changes in v2:
 - Add 3 new patches removing of_* API usage
 - Add MODULE_AUTHOR and MODULE_DESCRIPTION

Samuel Holland (4):
  gpio: sifive: Directly use the device's fwnode
  gpio: sifive: Look up IRQs only once during probe
  gpio: sifive: Get the parent IRQ's domain from its irq_data
  gpio: sifive: Allow building the driver as a module

 drivers/gpio/Kconfig       |  2 +-
 drivers/gpio/gpio-sifive.c | 45 +++++++++++++-------------------------
 2 files changed, 16 insertions(+), 31 deletions(-)

-- 
2.40.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v2 1/4] gpio: sifive: Directly use the device's fwnode

[Samuel Holland]

There is no need to convert dev->of_node back to a fwnode_handle.

Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

Changes in v2:
 - New patch for v2

 drivers/gpio/gpio-sifive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c
index 745e5f67254e..ab32c952c61b 100644
--- a/drivers/gpio/gpio-sifive.c
+++ b/drivers/gpio/gpio-sifive.c
@@ -254,7 +254,7 @@ static int sifive_gpio_probe(struct platform_device *pdev)
 	chip->gc.owner = THIS_MODULE;
 	girq = &chip->gc.irq;
 	gpio_irq_chip_set_chip(girq, &sifive_gpio_irqchip);
-	girq->fwnode = of_node_to_fwnode(node);
+	girq->fwnode = dev->fwnode;
 	girq->parent_domain = parent;
 	girq->child_to_parent_hwirq = sifive_gpio_child_to_parent_hwirq;
 	girq->handler = handle_bad_irq;
-- 
2.40.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 1/4] gpio: sifive: Directly use the device's fwnode

[Andy Shevchenko]

On Wed, Jul 19, 2023 at 09:34:42AM -0700, Samuel Holland wrote:
> There is no need to convert dev->of_node back to a fwnode_handle.

...

> -	girq->fwnode = of_node_to_fwnode(node);
> +	girq->fwnode = dev->fwnode;

	dev_fwnode(dev)

...and include property.h here

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v2 2/4] gpio: sifive: Look up IRQs only once during probe

[Samuel Holland]

of_irq_count(), or eqivalently platform_irq_count(), simply looks up
successively-numbered IRQs until that fails. Since this driver needs to
look up each IRQ anyway to get its virq number, use that existing loop
to count the IRQs at the same time.

Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

Changes in v2:
 - New patch for v2

 drivers/gpio/gpio-sifive.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c
index ab32c952c61b..6606c919d957 100644
--- a/drivers/gpio/gpio-sifive.c
+++ b/drivers/gpio/gpio-sifive.c
@@ -185,7 +185,7 @@ static int sifive_gpio_probe(struct platform_device *pdev)
 	struct irq_domain *parent;
 	struct gpio_irq_chip *girq;
 	struct sifive_gpio *chip;
-	int ret, ngpio, i;
+	int ret, ngpio;
 
 	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
@@ -202,13 +202,6 @@ static int sifive_gpio_probe(struct platform_device *pdev)
 	if (IS_ERR(chip->regs))
 		return PTR_ERR(chip->regs);
 
-	ngpio = of_irq_count(node);
-	if (ngpio > SIFIVE_GPIO_MAX) {
-		dev_err(dev, "Too many GPIO interrupts (max=%d)\n",
-			SIFIVE_GPIO_MAX);
-		return -ENXIO;
-	}
-
 	irq_parent = of_irq_find_parent(node);
 	if (!irq_parent) {
 		dev_err(dev, "no IRQ parent node\n");
@@ -221,11 +214,11 @@ static int sifive_gpio_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	for (i = 0; i < ngpio; i++) {
-		ret = platform_get_irq(pdev, i);
+	for (ngpio = 0; ngpio < SIFIVE_GPIO_MAX; ngpio++) {
+		ret = platform_get_irq_optional(pdev, ngpio);
 		if (ret < 0)
-			return ret;
-		chip->irq_number[i] = ret;
+			break;
+		chip->irq_number[ngpio] = ret;
 	}
 
 	ret = bgpio_init(&chip->gc, dev, 4,
-- 
2.40.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 2/4] gpio: sifive: Look up IRQs only once during probe

[Andy Shevchenko]

On Wed, Jul 19, 2023 at 09:34:43AM -0700, Samuel Holland wrote:
> of_irq_count(), or eqivalently platform_irq_count(), simply looks up
> successively-numbered IRQs until that fails. Since this driver needs to
> look up each IRQ anyway to get its virq number, use that existing loop
> to count the IRQs at the same time.

...

> -	ngpio = of_irq_count(node);
> -	if (ngpio > SIFIVE_GPIO_MAX) {
> -		dev_err(dev, "Too many GPIO interrupts (max=%d)\n",
> -			SIFIVE_GPIO_MAX);
> -		return -ENXIO;

Do we still need this check?

> -	}

...

> +	for (ngpio = 0; ngpio < SIFIVE_GPIO_MAX; ngpio++) {
> +		ret = platform_get_irq_optional(pdev, ngpio);
>  		if (ret < 0)
> -			return ret;
> -		chip->irq_number[i] = ret;
> +			break;
> +		chip->irq_number[ngpio] = ret;
>  	}

If so, here we need something like

	ret = platform_get_irq_optional(pdev, ngpio);
	if (ret > 0) {
		dev_err(dev, "Too many GPIO interrupts (max=%d)\n",
			SIFIVE_GPIO_MAX);
		return -ENXIO;
	}

Otherwise you need to mention this relaxation in the commit message.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 2/4] gpio: sifive: Look up IRQs only once during probe

[Samuel Holland]

On 2023-07-19 11:48 AM, Andy Shevchenko wrote:
> On Wed, Jul 19, 2023 at 09:34:43AM -0700, Samuel Holland wrote:
>> of_irq_count(), or eqivalently platform_irq_count(), simply looks up
>> successively-numbered IRQs until that fails. Since this driver needs to
>> look up each IRQ anyway to get its virq number, use that existing loop
>> to count the IRQs at the same time.
> 
> ...
> 
>> -	ngpio = of_irq_count(node);
>> -	if (ngpio > SIFIVE_GPIO_MAX) {
>> -		dev_err(dev, "Too many GPIO interrupts (max=%d)\n",
>> -			SIFIVE_GPIO_MAX);
>> -		return -ENXIO;
> 
> Do we still need this check?

I don't think so. I think this check was only intended to prevent overflowing
the irq_number array, and that is now handled by the loop condition.

>> -	}
> 
> ...
> 
>> +	for (ngpio = 0; ngpio < SIFIVE_GPIO_MAX; ngpio++) {
>> +		ret = platform_get_irq_optional(pdev, ngpio);
>>  		if (ret < 0)
>> -			return ret;
>> -		chip->irq_number[i] = ret;
>> +			break;
>> +		chip->irq_number[ngpio] = ret;
>>  	}
> 
> If so, here we need something like
> 
> 	ret = platform_get_irq_optional(pdev, ngpio);
> 	if (ret > 0) {
> 		dev_err(dev, "Too many GPIO interrupts (max=%d)\n",
> 			SIFIVE_GPIO_MAX);
> 		return -ENXIO;
> 	}
> 
> Otherwise you need to mention this relaxation in the commit message.

OK, I will add something to the commit message in v3.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v2 3/4] gpio: sifive: Get the parent IRQ's domain from its irq_data

[Samuel Holland]

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(-)

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;
-- 
2.40.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 3/4] gpio: sifive: Get the parent IRQ's domain from its irq_data

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 3/4] gpio: sifive: Get the parent IRQ's domain from its irq_data

[Samuel Holland]

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().


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 3/4] gpio: sifive: Get the parent IRQ's domain from its irq_data

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v2 4/4] gpio: sifive: Allow building the driver as a module

[Samuel Holland]

This can reduce the kernel image size in multiplatform configurations.

Acked-by: Palmer Dabbelt <palmer@rivosinc.com>
Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

Changes in v2:
 - Add MODULE_AUTHOR and MODULE_DESCRIPTION

 drivers/gpio/Kconfig       | 2 +-
 drivers/gpio/gpio-sifive.c | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index e382dfebad7c..1a8e8a8c85d6 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -564,7 +564,7 @@ config GPIO_SAMA5D2_PIOBU
 	  maintain their value during backup/self-refresh.
 
 config GPIO_SIFIVE
-	bool "SiFive GPIO support"
+	tristate "SiFive GPIO support"
 	depends on OF_GPIO
 	select IRQ_DOMAIN_HIERARCHY
 	select GPIO_GENERIC
diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c
index 46a42109d6f5..eacd67982de0 100644
--- a/drivers/gpio/gpio-sifive.c
+++ b/drivers/gpio/gpio-sifive.c
@@ -258,4 +258,8 @@ static struct platform_driver sifive_gpio_driver = {
 		.of_match_table = sifive_gpio_match,
 	},
 };
-builtin_platform_driver(sifive_gpio_driver)
+module_platform_driver(sifive_gpio_driver)
+
+MODULE_AUTHOR("Yash Shah <yash.shah@sifive.com>");
+MODULE_DESCRIPTION("SiFive GPIO driver");
+MODULE_LICENSE("GPL");
-- 
2.40.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 4/4] gpio: sifive: Allow building the driver as a module

[Andy Shevchenko]

On Wed, Jul 19, 2023 at 09:34:45AM -0700, Samuel Holland wrote:
> This can reduce the kernel image size in multiplatform configurations.

Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv