LinuxLists.cc - [PATCH 0/2] Make platform_get_irq[_byname]

2022-01-10 19:54:54

Subject: [PATCH 0/2] Make platform_get_irq[_byname]_optional() optional

Here are 2 patches making platform_get_irq[_byname]_optional() return 0 on
IRQ not being found, thus aligning those with the other *_optional() APIs;
they are against the 'driver-core-linus' branch of GregKH's 'driver-core.git'
repo plus the patch disallowing IRQ0 in platform_get_irq() and its ilk:

https://lore.kernel.org/lkml/[email protected]/

That patch was against the 'driver-core-linus' branch, so had to do these 2
against that branch as well; tell me if I should rebase to 'driver-core-next'.

Sergey Shtylyov (2):
platform: make platform_get_irq_optional() optional
platform: make platform_get_irq_byname_optional() optional

drivers/base/platform.c | 69 ++++++++++++-------
drivers/char/ipmi/bt-bmc.c | 8 ++-
drivers/counter/interrupt-cnt.c | 4 +-
drivers/edac/xgene_edac.c | 2 +-
drivers/gpio/gpio-altera.c | 3 +-
drivers/gpio/gpio-mvebu.c | 2 +-
drivers/gpio/gpio-tqmx86.c | 2 +-
drivers/gpu/drm/lima/lima_device.c | 2 +-
drivers/i2c/busses/i2c-brcmstb.c | 8 +--
drivers/i2c/busses/i2c-ocores.c | 4 +-
drivers/mailbox/tegra-hsp.c | 4 +-
drivers/mmc/host/sh_mmcif.c | 4 +-
drivers/mtd/nand/raw/brcmnand/brcmnand.c | 4 +-
drivers/net/can/rcar/rcar_canfd.c | 4 +-
drivers/net/dsa/b53/b53_srab.c | 2 +-
drivers/net/ethernet/davicom/dm9000.c | 2 +-
drivers/net/ethernet/freescale/fec_main.c | 2 +-
drivers/net/ethernet/freescale/fec_ptp.c | 4 +-
.../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 4 +-
.../ethernet/stmicro/stmmac/stmmac_platform.c | 4 +-
.../pci/controller/dwc/pcie-designware-host.c | 2 +-
drivers/phy/renesas/phy-rcar-gen3-usb2.c | 4 +-
drivers/platform/chrome/cros_ec_lpc.c | 2 +-
drivers/platform/x86/intel/punit_ipc.c | 2 +-
drivers/power/supply/mp2629_charger.c | 4 +-
drivers/spi/spi-bcm-qspi.c | 2 +-
drivers/spi/spi-hisi-sfc-v3xx.c | 2 +-
drivers/spi/spi-mtk-nor.c | 3 +-
drivers/spi/spi-rspi.c | 8 +--
drivers/thermal/rcar_gen3_thermal.c | 2 +-
drivers/tty/serial/8250/8250_mtk.c | 4 +-
drivers/tty/serial/sh-sci.c | 6 +-
drivers/uio/uio_pdrv_genirq.c | 2 +-
drivers/usb/cdns3/cdns3-plat.c | 5 +-
drivers/usb/host/xhci-mtk.c | 2 +-
drivers/usb/mtu3/mtu3_core.c | 2 +-
drivers/vfio/platform/vfio_platform.c | 6 +-
sound/soc/dwc/dwc-i2s.c | 4 +-
38 files changed, 110 insertions(+), 90 deletions(-)

--
2.26.3

2022-01-10 19:55:04

by Sergey Shtylyov

[permalink] [raw]

Subject: [PATCH 1/2] platform: make platform_get_irq_optional() optional

This patch is based on the former Andy Shevchenko's patch:

https://lore.kernel.org/lkml/[email protected]/

Currently platform_get_irq_optional() returns an error code even if IRQ
resource simply has not been found. It prevents the callers from being
error code agnostic in their error handling:

ret = platform_get_irq_optional(...);
if (ret < 0 && ret != -ENXIO)
return ret; // respect deferred probe
if (ret > 0)
...we get an IRQ...

All other *_optional() APIs seem to return 0 or NULL in case an optional
resource is not available. Let's follow this good example, so that the
callers would look like:

ret = platform_get_irq_optional(...);
if (ret < 0)
return ret;
if (ret > 0)
...we get an IRQ...

Reported-by: Matthias Schiffer <[email protected]>
Signed-off-by: Sergey Shtylyov <[email protected]>
---
drivers/base/platform.c | 56 +++++++++++++++---------
drivers/char/ipmi/bt-bmc.c | 8 ++--
drivers/counter/interrupt-cnt.c | 4 +-
drivers/edac/xgene_edac.c | 2 +-
drivers/gpio/gpio-altera.c | 3 +-
drivers/gpio/gpio-mvebu.c | 2 +-
drivers/gpio/gpio-tqmx86.c | 2 +-
drivers/i2c/busses/i2c-brcmstb.c | 8 ++--
drivers/i2c/busses/i2c-ocores.c | 4 +-
drivers/mmc/host/sh_mmcif.c | 4 +-
drivers/mtd/nand/raw/brcmnand/brcmnand.c | 4 +-
drivers/net/ethernet/davicom/dm9000.c | 2 +-
drivers/net/ethernet/freescale/fec_ptp.c | 2 +-
drivers/phy/renesas/phy-rcar-gen3-usb2.c | 4 +-
drivers/platform/chrome/cros_ec_lpc.c | 2 +-
drivers/platform/x86/intel/punit_ipc.c | 2 +-
drivers/power/supply/mp2629_charger.c | 4 +-
drivers/spi/spi-hisi-sfc-v3xx.c | 2 +-
drivers/spi/spi-mtk-nor.c | 3 +-
drivers/thermal/rcar_gen3_thermal.c | 2 +-
drivers/tty/serial/8250/8250_mtk.c | 4 +-
drivers/tty/serial/sh-sci.c | 6 +--
drivers/uio/uio_pdrv_genirq.c | 2 +-
drivers/vfio/platform/vfio_platform.c | 6 ++-
sound/soc/dwc/dwc-i2s.c | 4 +-
25 files changed, 79 insertions(+), 63 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index fc5a933f3698..7c7b3638f02d 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -148,25 +148,7 @@ devm_platform_ioremap_resource_byname(struct platform_device *pdev,
EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource_byname);
#endif /* CONFIG_HAS_IOMEM */

-/**
- * platform_get_irq_optional - get an optional IRQ for a device
- * @dev: platform device
- * @num: IRQ number index
- *
- * Gets an IRQ for a platform device. Device drivers should check the return
- * value for errors so as to not pass a negative integer value to the
- * request_irq() APIs. This is the same as platform_get_irq(), except that it
- * does not print an error message if an IRQ can not be obtained.
- *
- * For example::
- *
- * int irq = platform_get_irq_optional(pdev, 0);
- * if (irq < 0)
- * return irq;
- *
- * Return: non-zero IRQ number on success, negative error number on failure.
- */
-int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
+static int __platform_get_irq(struct platform_device *dev, unsigned int num)
{
int ret;
#ifdef CONFIG_SPARC
@@ -235,6 +217,38 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
return -EINVAL;
return ret;
}
+
+/**
+ * platform_get_irq_optional - get an optional IRQ for a device
+ * @dev: platform device
+ * @num: IRQ number index
+ *
+ * Gets an IRQ for a platform device. Device drivers should check the return
+ * value for errors so as to not pass a negative integer value to the
+ * request_irq() APIs. This is the same as platform_get_irq(), except that it
+ * does not print an error message if an IRQ can not be obtained and returns
+ * 0 when IRQ resource has not been found.
+ *
+ * For example::
+ *
+ * int irq = platform_get_irq_optional(pdev, 0);
+ * if (irq < 0)
+ * return irq;
+ * if (irq > 0)
+ * ...we have IRQ line defined...
+ *
+ * Return: non-zero IRQ number on success, 0 if IRQ wasn't found, negative error
+ * number on failure.
+ */
+int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
+{
+ int ret;
+
+ ret = __platform_get_irq(dev, num);
+ if (ret == -ENXIO)
+ return 0;
+ return ret;
+}
EXPORT_SYMBOL_GPL(platform_get_irq_optional);

/**
@@ -258,7 +272,7 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
{
int ret;

- ret = platform_get_irq_optional(dev, num);
+ ret = __platform_get_irq(dev, num);
if (ret < 0 && ret != -EPROBE_DEFER)
dev_err(&dev->dev, "IRQ index %u not found\n", num);

@@ -276,7 +290,7 @@ int platform_irq_count(struct platform_device *dev)
{
int ret, nr = 0;

- while ((ret = platform_get_irq_optional(dev, nr)) >= 0)
+ while ((ret = __platform_get_irq(dev, nr)) >= 0)
nr++;

if (ret == -EPROBE_DEFER)
diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c
index 7450904e330a..fdc63bfa5be4 100644
--- a/drivers/char/ipmi/bt-bmc.c
+++ b/drivers/char/ipmi/bt-bmc.c
@@ -382,12 +382,14 @@ static int bt_bmc_config_irq(struct bt_bmc *bt_bmc,
bt_bmc->irq = platform_get_irq_optional(pdev, 0);
if (bt_bmc->irq < 0)
return bt_bmc->irq;
+ if (!bt_bmc->irq)
+ return 0;

rc = devm_request_irq(dev, bt_bmc->irq, bt_bmc_irq, IRQF_SHARED,
DEVICE_NAME, bt_bmc);
if (rc < 0) {
dev_warn(dev, "Unable to request IRQ %d\n", bt_bmc->irq);
- bt_bmc->irq = rc;
+ bt_bmc->irq = 0;
return rc;
}

@@ -438,7 +440,7 @@ static int bt_bmc_probe(struct platform_device *pdev)

bt_bmc_config_irq(bt_bmc, pdev);

- if (bt_bmc->irq >= 0) {
+ if (bt_bmc->irq > 0) {
dev_info(dev, "Using IRQ %d\n", bt_bmc->irq);
} else {
dev_info(dev, "No IRQ; using timer\n");
@@ -464,7 +466,7 @@ static int bt_bmc_remove(struct platform_device *pdev)
struct bt_bmc *bt_bmc = dev_get_drvdata(&pdev->dev);

misc_deregister(&bt_bmc->miscdev);
- if (bt_bmc->irq < 0)
+ if (bt_bmc->irq <= 0)
del_timer_sync(&bt_bmc->poll_timer);
return 0;
}
diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
index 8514a87fcbee..a0564c035961 100644
--- a/drivers/counter/interrupt-cnt.c
+++ b/drivers/counter/interrupt-cnt.c
@@ -156,9 +156,7 @@ static int interrupt_cnt_probe(struct platform_device *pdev)
return -ENOMEM;

priv->irq = platform_get_irq_optional(pdev, 0);
- if (priv->irq == -ENXIO)
- priv->irq = 0;
- else if (priv->irq < 0)
+ if (priv->irq < 0)
return dev_err_probe(dev, priv->irq, "failed to get IRQ\n");

priv->gpio = devm_gpiod_get_optional(dev, NULL, GPIOD_IN);
diff --git a/drivers/edac/xgene_edac.c b/drivers/edac/xgene_edac.c
index 2ccd1db5e98f..0d1bdd27cd78 100644
--- a/drivers/edac/xgene_edac.c
+++ b/drivers/edac/xgene_edac.c
@@ -1917,7 +1917,7 @@ static int xgene_edac_probe(struct platform_device *pdev)

for (i = 0; i < 3; i++) {
irq = platform_get_irq_optional(pdev, i);
- if (irq < 0) {
+ if (irq <= 0) {
dev_err(&pdev->dev, "No IRQ resource\n");
rc = -EINVAL;
goto out_err;
diff --git a/drivers/gpio/gpio-altera.c b/drivers/gpio/gpio-altera.c
index b59fae993626..02a2995aa368 100644
--- a/drivers/gpio/gpio-altera.c
+++ b/drivers/gpio/gpio-altera.c
@@ -267,8 +267,7 @@ static int altera_gpio_probe(struct platform_device *pdev)
altera_gc->mmchip.gc.parent = &pdev->dev;

altera_gc->mapped_irq = platform_get_irq_optional(pdev, 0);
-
- if (altera_gc->mapped_irq < 0)
+ if (altera_gc->mapped_irq <= 0)
goto skip_irq;

if (of_property_read_u32(node, "altr,interrupt-type", &reg)) {
diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index 8f429d9f3661..a72a7bfc5a92 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -1294,7 +1294,7 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
for (i = 0; i < 4; i++) {
int irq = platform_get_irq_optional(pdev, i);

- if (irq < 0)
+ if (irq <= 0)
continue;
irq_set_chained_handler_and_data(irq, mvebu_gpio_irq_handler,
mvchip);
diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
index 5b103221b58d..dc0f83236ce8 100644
--- a/drivers/gpio/gpio-tqmx86.c
+++ b/drivers/gpio/gpio-tqmx86.c
@@ -237,7 +237,7 @@ static int tqmx86_gpio_probe(struct platform_device *pdev)
int ret, irq;

irq = platform_get_irq_optional(pdev, 0);
- if (irq < 0 && irq != -ENXIO)
+ if (irq < 0)
return irq;

res = platform_get_resource(pdev, IORESOURCE_IO, 0);
diff --git a/drivers/i2c/busses/i2c-brcmstb.c b/drivers/i2c/busses/i2c-brcmstb.c
index 490ee3962645..69395ae27a1a 100644
--- a/drivers/i2c/busses/i2c-brcmstb.c
+++ b/drivers/i2c/busses/i2c-brcmstb.c
@@ -250,7 +250,7 @@ static int brcmstb_i2c_wait_for_completion(struct brcmstb_i2c_dev *dev)
int ret = 0;
unsigned long timeout = msecs_to_jiffies(I2C_TIMEOUT);

- if (dev->irq >= 0) {
+ if (dev->irq > 0) {
if (!wait_for_completion_timeout(&dev->done, timeout))
ret = -ETIMEDOUT;
} else {
@@ -297,7 +297,7 @@ static int brcmstb_send_i2c_cmd(struct brcmstb_i2c_dev *dev,
return rc;

/* only if we are in interrupt mode */
- if (dev->irq >= 0)
+ if (dev->irq > 0)
reinit_completion(&dev->done);

/* enable BSC CTL interrupt line */
@@ -652,7 +652,7 @@ static int brcmstb_i2c_probe(struct platform_device *pdev)
brcmstb_i2c_enable_disable_irq(dev, INT_DISABLE);

/* register the ISR handler */
- if (dev->irq >= 0) {
+ if (dev->irq > 0) {
rc = devm_request_irq(&pdev->dev, dev->irq, brcmstb_i2c_isr,
IRQF_SHARED,
int_name ? int_name : pdev->name,
@@ -696,7 +696,7 @@ static int brcmstb_i2c_probe(struct platform_device *pdev)

dev_info(dev->device, "%s@%dhz registered in %s mode\n",
int_name ? int_name : " ", dev->clk_freq_hz,
- (dev->irq >= 0) ? "interrupt" : "polling");
+ (dev->irq > 0) ? "interrupt" : "polling");

return 0;

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index a0af027db04c..1f4d5e52ff42 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -691,10 +691,10 @@ static int ocores_i2c_probe(struct platform_device *pdev)
if (of_device_is_compatible(pdev->dev.of_node,
"sifive,fu540-c000-i2c")) {
i2c->flags |= OCORES_FLAG_BROKEN_IRQ;
- irq = -ENXIO;
+ irq = 0;
}

- if (irq == -ENXIO) {
+ if (!irq) {
ocores_algorithm.master_xfer = ocores_xfer_polling;
} else {
if (irq < 0)
diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index bcc595c70a9f..f558b9862032 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -1465,14 +1465,14 @@ static int sh_mmcif_probe(struct platform_device *pdev)
sh_mmcif_sync_reset(host);
sh_mmcif_writel(host->addr, MMCIF_CE_INT_MASK, MASK_ALL);

- name = irq[1] < 0 ? dev_name(dev) : "sh_mmc:error";
+ name = irq[1] <= 0 ? dev_name(dev) : "sh_mmc:error";
ret = devm_request_threaded_irq(dev, irq[0], sh_mmcif_intr,
sh_mmcif_irqt, 0, name, host);
if (ret) {
dev_err(dev, "request_irq error (%s)\n", name);
goto err_clk;
}
- if (irq[1] >= 0) {
+ if (irq[1] > 0) {
ret = devm_request_threaded_irq(dev, irq[1],
sh_mmcif_intr, sh_mmcif_irqt,
0, "sh_mmc:int", host);
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index f75929783b94..ac222985efde 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -1521,7 +1521,7 @@ static irqreturn_t brcmnand_ctlrdy_irq(int irq, void *data)

/* check if you need to piggy back on the ctrlrdy irq */
if (ctrl->edu_pending) {
- if (irq == ctrl->irq && ((int)ctrl->edu_irq >= 0))
+ if (irq == ctrl->irq && ((int)ctrl->edu_irq > 0))
/* Discard interrupts while using dedicated edu irq */
return IRQ_HANDLED;

@@ -2956,7 +2956,7 @@ static int brcmnand_edu_setup(struct platform_device *pdev)
brcmnand_edu_init(ctrl);

ctrl->edu_irq = platform_get_irq_optional(pdev, 1);
- if (ctrl->edu_irq < 0) {
+ if (ctrl->edu_irq <= 0) {
dev_warn(dev,
"FLASH EDU enabled, using ctlrdy irq\n");
} else {
diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
index 0985ab216566..740c660a9411 100644
--- a/drivers/net/ethernet/davicom/dm9000.c
+++ b/drivers/net/ethernet/davicom/dm9000.c
@@ -1509,7 +1509,7 @@ dm9000_probe(struct platform_device *pdev)
}

db->irq_wake = platform_get_irq_optional(pdev, 1);
- if (db->irq_wake >= 0) {
+ if (db->irq_wake > 0) {
dev_dbg(db->dev, "wakeup irq %d\n", db->irq_wake);

ret = request_irq(db->irq_wake, dm9000_wol_interrupt,
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index d71eac7e1924..158676eda48d 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -620,7 +620,7 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
/* Failure to get an irq is not fatal,
* only the PTP_CLOCK_PPS clock events should stop
*/
- if (irq >= 0) {
+ if (irq > 0) {
ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
0, pdev->name, ndev);
if (ret < 0)
diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
index 9de617ca9daa..4914d6aca208 100644
--- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
+++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
@@ -439,7 +439,7 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
u32 val;
int ret;

- if (!rcar_gen3_is_any_rphy_initialized(channel) && channel->irq >= 0) {
+ if (!rcar_gen3_is_any_rphy_initialized(channel) && channel->irq > 0) {
INIT_WORK(&channel->work, rcar_gen3_phy_usb2_work);
ret = request_irq(channel->irq, rcar_gen3_phy_usb2_irq,
IRQF_SHARED, dev_name(channel->dev), channel);
@@ -486,7 +486,7 @@ static int rcar_gen3_phy_usb2_exit(struct phy *p)
val &= ~USB2_INT_ENABLE_UCOM_INTEN;
writel(val, usb2_base + USB2_INT_ENABLE);

- if (channel->irq >= 0 && !rcar_gen3_is_any_rphy_initialized(channel))
+ if (channel->irq > 0 && !rcar_gen3_is_any_rphy_initialized(channel))
free_irq(channel->irq, channel);

return 0;
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index d6306d2a096f..91686d306534 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -400,7 +400,7 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
irq = platform_get_irq_optional(pdev, 0);
if (irq > 0)
ec_dev->irq = irq;
- else if (irq != -ENXIO) {
+ else if (irq < 0) {
dev_err(dev, "couldn't retrieve IRQ number (%d)\n", irq);
return irq;
}
diff --git a/drivers/platform/x86/intel/punit_ipc.c b/drivers/platform/x86/intel/punit_ipc.c
index 66bb39fd0ef9..f3cf5ee1466f 100644
--- a/drivers/platform/x86/intel/punit_ipc.c
+++ b/drivers/platform/x86/intel/punit_ipc.c
@@ -278,7 +278,7 @@ static int intel_punit_ipc_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, punit_ipcdev);

irq = platform_get_irq_optional(pdev, 0);
- if (irq < 0) {
+ if (irq <= 0) {
dev_warn(&pdev->dev, "Invalid IRQ, using polling mode\n");
} else {
ret = devm_request_irq(&pdev->dev, irq, intel_punit_ioc,
diff --git a/drivers/power/supply/mp2629_charger.c b/drivers/power/supply/mp2629_charger.c
index bdf924b73e47..51289700a7ac 100644
--- a/drivers/power/supply/mp2629_charger.c
+++ b/drivers/power/supply/mp2629_charger.c
@@ -581,9 +581,9 @@ static int mp2629_charger_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, charger);

irq = platform_get_irq_optional(to_platform_device(dev->parent), 0);
- if (irq < 0) {
+ if (irq <= 0) {
dev_err(dev, "get irq fail: %d\n", irq);
- return irq;
+ return irq < 0 ? irq : -ENXIO;
}

for (i = 0; i < MP2629_MAX_FIELD; i++) {
diff --git a/drivers/spi/spi-hisi-sfc-v3xx.c b/drivers/spi/spi-hisi-sfc-v3xx.c
index d3a23b1c2a4c..476ddc081c60 100644
--- a/drivers/spi/spi-hisi-sfc-v3xx.c
+++ b/drivers/spi/spi-hisi-sfc-v3xx.c
@@ -467,7 +467,7 @@ static int hisi_sfc_v3xx_probe(struct platform_device *pdev)
dev_err(dev, "failed to request irq%d, ret = %d\n", host->irq, ret);
host->irq = 0;
}
- } else {
+ } else if (host->irq < 0) {
host->irq = 0;
}

diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index 5c93730615f8..2422b0545936 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -829,8 +829,7 @@ static int mtk_nor_probe(struct platform_device *pdev)
mtk_nor_init(sp);

irq = platform_get_irq_optional(pdev, 0);
-
- if (irq < 0) {
+ if (irq <= 0) {
dev_warn(sp->dev, "IRQ not available.");
} else {
ret = devm_request_irq(sp->dev, irq, mtk_nor_irq_handler, 0,
diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 43eb25b167bc..776cfed4339c 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -430,7 +430,7 @@ static int rcar_gen3_thermal_request_irqs(struct rcar_gen3_thermal_priv *priv,

for (i = 0; i < 2; i++) {
irq = platform_get_irq_optional(pdev, i);
- if (irq < 0)
+ if (irq <= 0)
return irq;

irqname = devm_kasprintf(dev, GFP_KERNEL, "%s:ch%d",
diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
index fb65dc601b23..328ab074fd89 100644
--- a/drivers/tty/serial/8250/8250_mtk.c
+++ b/drivers/tty/serial/8250/8250_mtk.c
@@ -621,7 +621,7 @@ static int __maybe_unused mtk8250_suspend(struct device *dev)
serial8250_suspend_port(data->line);

pinctrl_pm_select_sleep_state(dev);
- if (irq >= 0) {
+ if (irq > 0) {
err = enable_irq_wake(irq);
if (err) {
dev_err(dev,
@@ -641,7 +641,7 @@ static int __maybe_unused mtk8250_resume(struct device *dev)
struct mtk8250_data *data = dev_get_drvdata(dev);
int irq = data->rx_wakeup_irq;

- if (irq >= 0)
+ if (irq > 0)
disable_irq_wake(irq);
pinctrl_pm_select_default_state(dev);

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 89ee43061d3a..a67f8e532a73 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1926,7 +1926,7 @@ static int sci_request_irq(struct sci_port *port)
* Certain port types won't support all of the
* available interrupt sources.
*/
- if (unlikely(irq < 0))
+ if (unlikely(irq <= 0))
continue;
}

@@ -1974,7 +1974,7 @@ static void sci_free_irq(struct sci_port *port)
* Certain port types won't support all of the available
* interrupt sources.
*/
- if (unlikely(irq < 0))
+ if (unlikely(irq <= 0))
continue;

/* Check if already freed (irq was muxed) */
@@ -2901,7 +2901,7 @@ static int sci_init_single(struct platform_device *dev,
if (sci_port->irqs[0] < 0)
return -ENXIO;

- if (sci_port->irqs[1] < 0)
+ if (sci_port->irqs[1] <= 0)
for (i = 1; i < ARRAY_SIZE(sci_port->irqs); i++)
sci_port->irqs[i] = sci_port->irqs[0];

diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index 63258b6accc4..7fd275fc6ceb 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -162,7 +162,7 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
if (!uioinfo->irq) {
ret = platform_get_irq_optional(pdev, 0);
uioinfo->irq = ret;
- if (ret == -ENXIO)
+ if (!ret)
uioinfo->irq = UIO_IRQ_NONE;
else if (ret == -EPROBE_DEFER)
return ret;
diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
index 68a1c87066d7..cd7494933563 100644
--- a/drivers/vfio/platform/vfio_platform.c
+++ b/drivers/vfio/platform/vfio_platform.c
@@ -32,8 +32,12 @@ static struct resource *get_platform_resource(struct vfio_platform_device *vdev,
static int get_platform_irq(struct vfio_platform_device *vdev, int i)
{
struct platform_device *pdev = (struct platform_device *) vdev->opaque;
+ int ret;

- return platform_get_irq_optional(pdev, i);
+ ret = platform_get_irq_optional(pdev, i);
+ if (ret < 0)
+ return ret;
+ return ret > 0 ? ret : -ENXIO;
}

static int vfio_platform_probe(struct platform_device *pdev)
diff --git a/sound/soc/dwc/dwc-i2s.c b/sound/soc/dwc/dwc-i2s.c
index 5cb58929090d..ff19c5130459 100644
--- a/sound/soc/dwc/dwc-i2s.c
+++ b/sound/soc/dwc/dwc-i2s.c
@@ -643,7 +643,7 @@ static int dw_i2s_probe(struct platform_device *pdev)
dev->dev = &pdev->dev;

irq = platform_get_irq_optional(pdev, 0);
- if (irq >= 0) {
+ if (irq > 0) {
ret = devm_request_irq(&pdev->dev, irq, i2s_irq_handler, 0,
pdev->name, dev);
if (ret < 0) {
@@ -697,7 +697,7 @@ static int dw_i2s_probe(struct platform_device *pdev)
}

if (!pdata) {
- if (irq >= 0) {
+ if (irq > 0) {
ret = dw_pcm_register(pdev);
dev->use_pio = true;
} else {
--
2.26.3

2022-01-10 19:55:10

by Sergey Shtylyov

[permalink] [raw]

Subject: [PATCH 2/2] platform: make platform_get_irq_byname_optional() optional

Currently platform_get_irq_byname_optional() returns an error code even
if IRQ resource simply has not been found. It prevents the callers from
being error code agnostic in their error handling:

ret = platform_get_irq_byname_optional(...);
if (ret < 0 && ret != -ENXIO)
return ret; // respect deferred probe
if (ret > 0)
...we get an IRQ...

All other *_optional() APIs seem to return 0 or NULL in case an optional
resource is not available. Let's follow this good example, so that the
callers would look like:

ret = platform_get_irq_byname_optional(...);
if (ret < 0)
return ret;
if (ret > 0)
...we get an IRQ...

Signed-off-by: Sergey Shtylyov <[email protected]>
---
drivers/base/platform.c | 13 ++++++++++---
drivers/gpu/drm/lima/lima_device.c | 2 +-
drivers/mailbox/tegra-hsp.c | 4 ++--
drivers/net/can/rcar/rcar_canfd.c | 4 ++--
drivers/net/dsa/b53/b53_srab.c | 2 +-
drivers/net/ethernet/freescale/fec_main.c | 2 +-
drivers/net/ethernet/freescale/fec_ptp.c | 2 +-
drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c | 4 ++--
.../net/ethernet/stmicro/stmmac/stmmac_platform.c | 4 ++--
drivers/pci/controller/dwc/pcie-designware-host.c | 2 +-
drivers/spi/spi-bcm-qspi.c | 2 +-
drivers/spi/spi-rspi.c | 8 ++++----
drivers/usb/cdns3/cdns3-plat.c | 5 +----
drivers/usb/host/xhci-mtk.c | 2 +-
drivers/usb/mtu3/mtu3_core.c | 2 +-
15 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 7c7b3638f02d..1d0ea635922b 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -495,14 +495,21 @@ EXPORT_SYMBOL_GPL(platform_get_irq_byname);
* @name: IRQ name
*
* Get an optional IRQ by name like platform_get_irq_byname(). Except that it
- * does not print an error message if an IRQ can not be obtained.
+ * does not print an error message if an IRQ can not be obtained and returns
+ * 0 when IRQ resource has not been found.
*
- * Return: non-zero IRQ number on success, negative error number on failure.
+ * Return: non-zero IRQ number on success, 0 if IRQ wasn't found, negative error
+ * number on failure.
*/
int platform_get_irq_byname_optional(struct platform_device *dev,
const char *name)
{
- return __platform_get_irq_byname(dev, name);
+ int ret;
+
+ ret = __platform_get_irq_byname(dev, name);
+ if (ret == -ENXIO)
+ return 0;
+ return ret;
}
EXPORT_SYMBOL_GPL(platform_get_irq_byname_optional);

diff --git a/drivers/gpu/drm/lima/lima_device.c b/drivers/gpu/drm/lima/lima_device.c
index 65fdca366e41..e3659aa687c2 100644
--- a/drivers/gpu/drm/lima/lima_device.c
+++ b/drivers/gpu/drm/lima/lima_device.c
@@ -223,7 +223,7 @@ static int lima_init_ip(struct lima_device *dev, int index)
if (irq_name) {
err = must ? platform_get_irq_byname(pdev, irq_name) :
platform_get_irq_byname_optional(pdev, irq_name);
- if (err < 0)
+ if (err <= 0)
goto out;
ip->irq = err;
}
diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c
index acd0675da681..17aa88e31445 100644
--- a/drivers/mailbox/tegra-hsp.c
+++ b/drivers/mailbox/tegra-hsp.c
@@ -667,7 +667,7 @@ static int tegra_hsp_probe(struct platform_device *pdev)
hsp->num_si = (value >> HSP_nSI_SHIFT) & HSP_nINT_MASK;

err = platform_get_irq_byname_optional(pdev, "doorbell");
- if (err >= 0)
+ if (err > 0)
hsp->doorbell_irq = err;

if (hsp->num_si > 0) {
@@ -687,7 +687,7 @@ static int tegra_hsp_probe(struct platform_device *pdev)
return -ENOMEM;

err = platform_get_irq_byname_optional(pdev, name);
- if (err >= 0) {
+ if (err > 0) {
hsp->shared_irqs[i] = err;
count++;
}
diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index ff9d0f5ae0dd..1d4794493c6a 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -1778,7 +1778,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)

if (chip_id == RENESAS_RCAR_GEN3) {
ch_irq = platform_get_irq_byname_optional(pdev, "ch_int");
- if (ch_irq < 0) {
+ if (ch_irq <= 0) {
/* For backward compatibility get irq by index */
ch_irq = platform_get_irq(pdev, 0);
if (ch_irq < 0)
@@ -1786,7 +1786,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
}

g_irq = platform_get_irq_byname_optional(pdev, "g_int");
- if (g_irq < 0) {
+ if (g_irq <= 0) {
/* For backward compatibility get irq by index */
g_irq = platform_get_irq(pdev, 1);
if (g_irq < 0)
diff --git a/drivers/net/dsa/b53/b53_srab.c b/drivers/net/dsa/b53/b53_srab.c
index 4591bb1c05d2..80b7c8f053ad 100644
--- a/drivers/net/dsa/b53/b53_srab.c
+++ b/drivers/net/dsa/b53/b53_srab.c
@@ -420,7 +420,7 @@ static int b53_srab_irq_enable(struct b53_device *dev, int port)
/* Interrupt is optional and was not specified, do not make
* this fatal
*/
- if (p->irq == -ENXIO)
+ if (!p->irq)
return ret;

ret = request_threaded_irq(p->irq, b53_srab_port_isr,
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index bc418b910999..fba36d09a6e0 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -3933,7 +3933,7 @@ fec_probe(struct platform_device *pdev)
for (i = 0; i < irq_cnt; i++) {
snprintf(irq_name, sizeof(irq_name), "int%d", i);
irq = platform_get_irq_byname_optional(pdev, irq_name);
- if (irq < 0)
+ if (irq <= 0)
irq = platform_get_irq(pdev, i);
if (irq < 0) {
ret = irq;
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 158676eda48d..251863c2d5a4 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -615,7 +615,7 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
INIT_DELAYED_WORK(&fep->time_keep, fec_time_keep);

irq = platform_get_irq_byname_optional(pdev, "pps");
- if (irq < 0)
+ if (irq <= 0)
irq = platform_get_irq_optional(pdev, irq_idx);
/* Failure to get an irq is not fatal,
* only the PTP_CLOCK_PPS clock events should stop
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
index 2b38a499a404..5519b5b35365 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
@@ -342,7 +342,7 @@ static int stm32mp1_parse_data(struct stm32_dwmac *dwmac,
if (dwmac->irq_pwr_wakeup == -EPROBE_DEFER)
return -EPROBE_DEFER;

- if (!dwmac->clk_eth_ck && dwmac->irq_pwr_wakeup >= 0) {
+ if (!dwmac->clk_eth_ck && dwmac->irq_pwr_wakeup > 0) {
err = device_init_wakeup(&pdev->dev, true);
if (err) {
dev_err(&pdev->dev, "Failed to init wake up irq\n");
@@ -426,7 +426,7 @@ static int stm32_dwmac_remove(struct platform_device *pdev)

stm32_dwmac_clk_disable(priv->plat->bsp_priv);

- if (dwmac->irq_pwr_wakeup >= 0) {
+ if (dwmac->irq_pwr_wakeup > 0) {
dev_pm_clear_wake_irq(&pdev->dev);
device_init_wakeup(&pdev->dev, false);
}
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 232ac98943cd..dcfc04f7bfd4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -679,7 +679,7 @@ int stmmac_get_platform_resources(struct platform_device *pdev,
*/
stmmac_res->wol_irq =
platform_get_irq_byname_optional(pdev, "eth_wake_irq");
- if (stmmac_res->wol_irq < 0) {
+ if (stmmac_res->wol_irq <= 0) {
if (stmmac_res->wol_irq == -EPROBE_DEFER)
return -EPROBE_DEFER;
dev_info(&pdev->dev, "IRQ eth_wake_irq not found\n");
@@ -688,7 +688,7 @@ int stmmac_get_platform_resources(struct platform_device *pdev,

stmmac_res->lpi_irq =
platform_get_irq_byname_optional(pdev, "eth_lpi");
- if (stmmac_res->lpi_irq < 0) {
+ if (stmmac_res->lpi_irq <= 0) {
if (stmmac_res->lpi_irq == -EPROBE_DEFER)
return -EPROBE_DEFER;
dev_info(&pdev->dev, "IRQ eth_lpi not found\n");
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index f4755f3a03be..00e1a33fd06d 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -364,7 +364,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
} else if (pp->has_msi_ctrl) {
if (!pp->msi_irq) {
pp->msi_irq = platform_get_irq_byname_optional(pdev, "msi");
- if (pp->msi_irq < 0) {
+ if (pp->msi_irq <= 0) {
pp->msi_irq = platform_get_irq(pdev, 0);
if (pp->msi_irq < 0)
return pp->msi_irq;
diff --git a/drivers/spi/spi-bcm-qspi.c b/drivers/spi/spi-bcm-qspi.c
index f3de3305d0f5..40ca101e9875 100644
--- a/drivers/spi/spi-bcm-qspi.c
+++ b/drivers/spi/spi-bcm-qspi.c
@@ -1595,7 +1595,7 @@ int bcm_qspi_probe(struct platform_device *pdev,
irq = platform_get_irq(pdev, 0);
}

- if (irq >= 0) {
+ if (irq > 0) {
ret = devm_request_irq(&pdev->dev, irq,
qspi_irq_tab[val].irq_handler, 0,
name,
diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
index 41761f0d892a..b736b57f5ff2 100644
--- a/drivers/spi/spi-rspi.c
+++ b/drivers/spi/spi-rspi.c
@@ -1330,16 +1330,16 @@ static int rspi_probe(struct platform_device *pdev)
ctlr->max_native_cs = rspi->ops->num_hw_ss;

ret = platform_get_irq_byname_optional(pdev, "rx");
- if (ret < 0) {
+ if (ret <= 0) {
ret = platform_get_irq_byname_optional(pdev, "mux");
- if (ret < 0)
+ if (ret <= 0)
ret = platform_get_irq(pdev, 0);
- if (ret >= 0)
+ if (ret > 0)
rspi->rx_irq = rspi->tx_irq = ret;
} else {
rspi->rx_irq = ret;
ret = platform_get_irq_byname(pdev, "tx");
- if (ret >= 0)
+ if (ret > 0)
rspi->tx_irq = ret;
}

diff --git a/drivers/usb/cdns3/cdns3-plat.c b/drivers/usb/cdns3/cdns3-plat.c
index 4d0f027e5bd3..7379b6026f9f 100644
--- a/drivers/usb/cdns3/cdns3-plat.c
+++ b/drivers/usb/cdns3/cdns3-plat.c
@@ -108,10 +108,7 @@ static int cdns3_plat_probe(struct platform_device *pdev)
cdns->wakeup_irq = platform_get_irq_byname_optional(pdev, "wakeup");
if (cdns->wakeup_irq == -EPROBE_DEFER)
return cdns->wakeup_irq;
- else if (cdns->wakeup_irq == 0)
- return -EINVAL;
-
- if (cdns->wakeup_irq < 0) {
+ if (cdns->wakeup_irq <= 0) {
dev_dbg(dev, "couldn't get wakeup irq\n");
cdns->wakeup_irq = 0x0;
}
diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
index 58a0eae4f41b..e3071e7cb165 100644
--- a/drivers/usb/host/xhci-mtk.c
+++ b/drivers/usb/host/xhci-mtk.c
@@ -495,7 +495,7 @@ static int xhci_mtk_probe(struct platform_device *pdev)
return ret;

irq = platform_get_irq_byname_optional(pdev, "host");
- if (irq < 0) {
+ if (irq <= 0) {
if (irq == -EPROBE_DEFER)
return irq;

diff --git a/drivers/usb/mtu3/mtu3_core.c b/drivers/usb/mtu3/mtu3_core.c
index c4a2c37abf62..08173c05a1d6 100644
--- a/drivers/usb/mtu3/mtu3_core.c
+++ b/drivers/usb/mtu3/mtu3_core.c
@@ -925,7 +925,7 @@ int ssusb_gadget_init(struct ssusb_mtk *ssusb)
return -ENOMEM;

mtu->irq = platform_get_irq_byname_optional(pdev, "device");
- if (mtu->irq < 0) {
+ if (mtu->irq <= 0) {
if (mtu->irq == -EPROBE_DEFER)
return mtu->irq;

--
2.26.3

2022-01-10 20:11:47

On Mon, Jan 10, 2022 at 11:07:03PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 10, 2022 at 09:10:14PM +0100, Uwe Kleine-K?nig wrote:
> > On Mon, Jan 10, 2022 at 10:54:48PM +0300, Sergey Shtylyov wrote:
> > > This patch is based on the former Andy Shevchenko's patch:
> > >
> > > https://lore.kernel.org/lkml/[email protected]/
> > >
> > > Currently platform_get_irq_optional() returns an error code even if IRQ
> > > resource simply has not been found. It prevents the callers from being
> > > error code agnostic in their error handling:
> > >
> > > ret = platform_get_irq_optional(...);
> > > if (ret < 0 && ret != -ENXIO)
> > > return ret; // respect deferred probe
> > > if (ret > 0)
> > > ...we get an IRQ...
> > >
> > > All other *_optional() APIs seem to return 0 or NULL in case an optional
> > > resource is not available. Let's follow this good example, so that the
> > > callers would look like:
> > >
> > > ret = platform_get_irq_optional(...);
> > > if (ret < 0)
> > > return ret;
> > > if (ret > 0)
> > > ...we get an IRQ...
> >
> > The difference to gpiod_get_optional (and most other *_optional) is that
> > you can use the NULL value as if it were a valid GPIO.
>
> The problem is not only there, but also in the platform_get_irq() and that
> problem is called vIRQ0. Or as Linus put it "_cookie_" for IRQ, which never
> ever should be 0.

IMHO it's best to avoid yielding zero for a value that should be
interpreted as an (virtual) irq. Then callers don't even have to
consider if it's a valid value or not.

> > As this isn't given with for irqs, I don't think changing the return
> > value has much sense. In my eyes the problem with platform_get_irq() and
> > platform_get_irq_optional() is that someone considered it was a good
> > idea that a global function emits an error message. The problem is,
> > that's only true most of the time. (Sometimes the caller can handle an
> > error (here: the absence of an irq) just fine, sometimes the generic
> > error message just isn't as good as a message by the caller could be.
> > (here: The caller could emit "TX irq not found" which is a much nicer
> > message than "IRQ index 5 not found".)
> >
> > My suggestion would be to keep the return value of
> > platform_get_irq_optional() as is, but rename it to
> > platform_get_irq_silent() to get rid of the expectation invoked by the
> > naming similarity that motivated you to change
> > platform_get_irq_optional().
>
> This won't fix the issue with vIRQ0.

Is the patch about vIRQ0, or did you only start to consider it when I
said that for gpio NULL is a dummy value? If the former, the commit log
should better mention that.

Anyhow, I still think renaming platform_get_irq_optional() to
platform_get_irq_silent() is a good idea and the patches in this thread
are not.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |

Attachments:

(No filename) (2.96 kB)
signature.asc (488.00 B)
Download all attachments

2022-01-12 08:34:08

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional

Hi Andrew,

On Mon, Jan 10, 2022 at 10:20 PM Andrew Lunn <[email protected]> wrote:
> On Mon, Jan 10, 2022 at 09:10:14PM +0100, Uwe Kleine-König wrote:
> > On Mon, Jan 10, 2022 at 10:54:48PM +0300, Sergey Shtylyov wrote:
> > > This patch is based on the former Andy Shevchenko's patch:
> > >
> > > https://lore.kernel.org/lkml/[email protected]/
> > >
> > > Currently platform_get_irq_optional() returns an error code even if IRQ
> > > resource simply has not been found. It prevents the callers from being
> > > error code agnostic in their error handling:
> > >
> > > ret = platform_get_irq_optional(...);
> > > if (ret < 0 && ret != -ENXIO)
> > > return ret; // respect deferred probe
> > > if (ret > 0)
> > > ...we get an IRQ...
> > >
> > > All other *_optional() APIs seem to return 0 or NULL in case an optional
> > > resource is not available. Let's follow this good example, so that the
> > > callers would look like:
> > >
> > > ret = platform_get_irq_optional(...);
> > > if (ret < 0)
> > > return ret;
> > > if (ret > 0)
> > > ...we get an IRQ...
> >
> > The difference to gpiod_get_optional (and most other *_optional) is that
> > you can use the NULL value as if it were a valid GPIO.
> >
> > As this isn't given with for irqs, I don't think changing the return
> > value has much sense.
>
> We actually want platform_get_irq_optional() to look different to all
> the other _optional() methods because it is not equivalent. If it
> looks the same, developers will assume it is the same, and get
> themselves into trouble.

Developers already assume it is the same, and thus forget they have
to check against -ENXIO instead of zero.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2022-01-12 08:54:06

On Fri, Jan 14, 2022 at 10:45:38PM +0300, Sergey Shtylyov wrote:
> On 1/13/22 10:43 PM, Uwe Kleine-K?nig wrote:
>
> > The subsystems regulator, clk and gpio have the concept of a dummy
> > resource. For regulator, clk and gpio there is a semantic difference
> > between the regular _get() function and the _get_optional() variant.
> > (One might return the dummy resource, the other won't. Unfortunately
> > which one implements which isn't the same for these three.) The
> > difference between platform_get_irq() and platform_get_irq_optional() is
> > only that the former might emit an error message and the later won't.
> >
> > To prevent people's expectations that there is a semantic difference
> > between these too, rename platform_get_irq_optional() to
> > platform_get_irq_silent() to make the actual difference more obvious.
> >
> > The #define for the old name can and should be removed once all patches
> > currently in flux still relying on platform_get_irq_optional() are
> > fixed.
>
> Hm... I'm afraid that with this #define they would never get fixed... :-)

I will care for it.

> > Signed-off-by: Uwe Kleine-K?nig <[email protected]>
> > ---
> > Hello,
> >
> > On Thu, Jan 13, 2022 at 02:45:30PM +0000, Mark Brown wrote:
> >> On Thu, Jan 13, 2022 at 12:08:31PM +0100, Uwe Kleine-K?nig wrote:
> >>
> >>> This is all very unfortunate. In my eyes b) is the most sensible
> >>> sense, but the past showed that we don't agree here. (The most annoying
> >>> part of regulator_get is the warning that is emitted that regularily
> >>> makes customers ask what happens here and if this is fixable.)
> >>
> >> Fortunately it can be fixed, and it's safer to clearly specify things.
> >> The prints are there because when the description is wrong enough to
> >> cause things to blow up we can fail to boot or run messily and
> >> forgetting to describe some supplies (or typoing so they haven't done
> >> that) and people were having a hard time figuring out what might've
> >> happened.
> >
> > Yes, that's right. I sent a patch for such a warning in 2019 and pinged
> > occationally. Still waiting for it to be merged :-\
> > (https://lore.kernel.org/r/[email protected])
> >
> >>> I think at least c) is easy to resolve because
> >>> platform_get_irq_optional() isn't that old yet and mechanically
> >>> replacing it by platform_get_irq_silent() should be easy and safe.
> >>> And this is orthogonal to the discussion if -ENOXIO is a sensible return
> >>> value and if it's as easy as it could be to work with errors on irq
> >>> lookups.
> >>
> >> It'd certainly be good to name anything that doesn't correspond to one
> >> of the existing semantics for the API (!) something different rather
> >> than adding yet another potentially overloaded meaning.
> >
> > It seems we're (at least) three who agree about this. Here is a patch
> > fixing the name.
>
> I can't say I genrally agree with this patch...

Yes, I didn't count you to the three people signaling agreement.

> [...]
> > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> > index 7c96f169d274..6d495f15f717 100644
> > --- a/include/linux/platform_device.h
> > +++ b/include/linux/platform_device.h
> > @@ -69,7 +69,14 @@ extern void __iomem *
> > devm_platform_ioremap_resource_byname(struct platform_device *pdev,
> > const char *name);
> > extern int platform_get_irq(struct platform_device *, unsigned int);
> > -extern int platform_get_irq_optional(struct platform_device *, unsigned int);
> > +extern int platform_get_irq_silent(struct platform_device *, unsigned int);
> > +
> > +/*
> > + * platform_get_irq_optional was recently renamed to platform_get_irq_silent.
> > + * Fixup users to not break patches that were created before the rename.
> > + */
> > +#define platform_get_irq_optional(pdev, index) platform_get_irq_silent(pdev, index)
> > +
>
> Yeah, why bother fixing if it compiles anyway?

The plan is to remove the define in one or two kernel releases. The idea
is only to not break patches that are currently in next.

> I think an inline wrapper with an indication to gcc that the function is deprecated
> (I just forgot how it should look) would be better instead...

The deprecated function annotation is generally frowned upon. See
771c035372a0.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |

Attachments:

(No filename) (4.52 kB)
signature.asc (499.00 B)
Download all attachments

2022-01-14 23:05:59

by Uwe Kleine-König

[permalink] [raw]

Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional

On Fri, Jan 14, 2022 at 10:14:10PM +0300, Sergey Shtylyov wrote:
> On 1/14/22 12:25 PM, Uwe Kleine-K?nig wrote:
>
> >>>>> To me it sounds much more logical for the driver to check if an
> >>>>> optional irq is non-zero (available) or zero (not available), than to
> >>>>> sprinkle around checks for -ENXIO. In addition, you have to remember
> >>>>> that this one returns -ENXIO, while other APIs use -ENOENT or -ENOSYS
> >>>>> (or some other error code) to indicate absence. I thought not having
> >>>>> to care about the actual error code was the main reason behind the
> >>>>> introduction of the *_optional() APIs.
> >>>
> >>>> No, the main benefit of gpiod_get_optional() (and clk_get_optional()) is
> >>>> that you can handle an absent GPIO (or clk) as if it were available.
> >>
> >> Hm, I've just looked at these and must note that they match 1:1 with
> >> platform_get_irq_optional(). Unfortunately, we can't however behave the
> >> same way in request_irq() -- because it has to support IRQ0 for the sake
> >> of i8253 drivers in arch/...
> >
> > Let me reformulate your statement to the IMHO equivalent:
> >
> > If you set aside the differences between
> > platform_get_irq_optional() and gpiod_get_optional(),
>
> Sorry, I should make it clear this is actually the diff between a would-be
> platform_get_irq_optional() after my patch, not the current code...

The similarity is that with your patch both gpiod_get_optional() and
platform_get_irq_optional() return NULL and 0 on not-found. The relevant
difference however is that for a gpiod NULL is a dummy value, while for
irqs it's not. So the similarity is only syntactically, but not
semantically.

> > platform_get_irq_optional() is like gpiod_get_optional().
> >
> > The introduction of gpiod_get_optional() made it possible to simplify
> > the following code:
> >
> > reset_gpio = gpiod_get(...)
> > if IS_ERR(reset_gpio):
> > error = PTR_ERR(reset_gpio)
> > if error != -ENDEV:
>
> ENODEV?

Yes, typo.

> > return error
> > else:
> > gpiod_set_direction(reset_gpiod, INACTIVE)
> >
> > to
> >
> > reset_gpio = gpiod_get_optional(....)
> > if IS_ERR(reset_gpio):
> > return reset_gpio
> > gpiod_set_direction(reset_gpiod, INACTIVE)
> >
> > and I never need to actually know if the reset_gpio actually exists.
> > Either the line is put into its inactive state, or it doesn't exist and
> > then gpiod_set_direction is a noop. For a regulator or a clk this works
> > in a similar way.
> >
> > However for an interupt this cannot work. You will always have to check
> > if the irq is actually there or not because if it's not you cannot just
> > ignore that. So there is no benefit of an optional irq.
> >
> > Leaving error message reporting aside, the introduction of
> > platform_get_irq_optional() allows to change
> >
> > irq = platform_get_irq(...);
> > if (irq < 0 && irq != -ENXIO) {
> > return irq;
> > } else if (irq >= 0) {
>
> Rather (irq > 0) actually, IRQ0 is considered invalid (but still returned).

This is a topic I don't feel strong for, so I'm sloppy here. If changing
this is all that is needed to convince you of my point ...

> > ... setup irq operation ...
> > } else { /* irq == -ENXIO */
> > ... setup polling ...
> > }
> >
> > to
> >
> > irq = platform_get_irq_optional(...);
> > if (irq < 0 && irq != -ENXIO) {
> > return irq;
> > } else if (irq >= 0) {
> > ... setup irq operation ...
> > } else { /* irq == -ENXIO */
> > ... setup polling ...
> > }
> >
> > which isn't a win. When changing the return value as you suggest, it can
> > be changed instead to:
> >
> > irq = platform_get_irq_optional(...);
> > if (irq < 0) {
> > return irq;
> > } else if (irq > 0) {
> > ... setup irq operation ...
> > } else { /* irq == 0 */
> > ... setup polling ...
> > }
> >
> > which is a tad nicer. If that is your goal however I ask you to also
> > change the semantic of platform_get_irq() to return 0 on "not found".
>
> Well, I'm not totally opposed to that... but would there be a considerable win?

Well, compared to your suggestion of making platform_get_irq_optional()
return 0 on "not-found" the considerable win would be that
platform_get_irq_optional() and platform_get_irq() are not different
just because platform_get_irq() is to hard to change.

> Anyway, we should 1st stop returning 0 for "valid" IRQs -- this is done by my patch
> the discussed patch series are atop of.
>
> > Note the win is considerably less compared to gpiod_get_optional vs
>
> If there's any at all... We'd basically have to touch /all/ platform_get_irq()
> calls (and get an even larger CC list ;-)).

You got me wrong here. I meant that even if you change both
platform_get_irq() and platform_get_irq_optional() to return 0 on
"not-found", the win is small compared to the benefit of having both
clk_get() and clk_get_optional().

> > gpiod_get however. And then it still lacks the semantic of a dummy irq
> > which IMHO forfeits the right to call it ..._optional().
>
> Not quite grasping it... Why e.g. clk_get() doesn't return 0 for a not found clock?

Because NULL is not an error value for clk and when calling clk_get()
you want a failure when the clk you asked for isn't available.

Sure you could do the following in a case where you want to insist the
clk to be actually available:

clk = clk_get_optional(...)
if (IS_ERR_OR_NULL(clk)) {
err = PTR_ERR(clk) || -ENODEV;
return dev_err_probe(dev, err, ....);
}

but this is more ugly than

clk = clk_get(...)
if (IS_ERR(clk)) {
err = PTR_ERR(clk);
return dev_err_probe(dev, err, ....);
}

Additionally the first usage would hard-code in the drivers that NULL is
the dummy value which you might want to consider a layer violation.

You have to understand that for clk (and regulator and gpiod) NULL is a
valid descriptor that can actually be used, it just has no effect. So
this is a convenience value for the case "If the clk/regulator/gpiod in
question isn't available, there is nothing to do". This is what makes
clk_get_optional() and the others really useful and justifies their
existence. This doesn't apply to platform_get_irq_optional().

So clk_get() is sane and sensible for cases where you need the clk to be
there. It doesn't emit an error message, because the caller knows better
if it's worth an error message and in some cases the caller can also
emit a better error message than clk_get() itself.

clk_get_optional() is sane and sensible for cases where the clk might be
absent and it helps you because you don't have to differentiate between
"not found" and "there is an actual resource".

The reason for platform_get_irq_optional()'s existence is just that
platform_get_irq() emits an error message which is wrong or suboptimal
in some cases (and IMHO is platform_get_irq() root fault). It doesn't
simplify handling the "not found" case. So let's not pretend by the
choice of function names that there is a similarity between clk_get() +
clk_get_optional() and platform_get_irq() + platform_get_irq_optional().

And as you cannot change platform_get_irq_optional() to return a working
dummy value, IMHO the only sane way out is renaming it.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |

Attachments:

(No filename) (7.39 kB)
signature.asc (499.00 B)
Download all attachments

2022-01-16 06:45:55

by Sergey Shtylyov

On 1/14/22 11:22 PM, Uwe Kleine-K?nig wrote:
> On Fri, Jan 14, 2022 at 10:14:10PM +0300, Sergey Shtylyov wrote:
>> On 1/14/22 12:25 PM, Uwe Kleine-K?nig wrote:
>>
>>>>>>> To me it sounds much more logical for the driver to check if an
>>>>>>> optional irq is non-zero (available) or zero (not available), than to
>>>>>>> sprinkle around checks for -ENXIO. In addition, you have to remember
>>>>>>> that this one returns -ENXIO, while other APIs use -ENOENT or -ENOSYS
>>>>>>> (or some other error code) to indicate absence. I thought not having
>>>>>>> to care about the actual error code was the main reason behind the
>>>>>>> introduction of the *_optional() APIs.
>>>>>
>>>>>> No, the main benefit of gpiod_get_optional() (and clk_get_optional()) is
>>>>>> that you can handle an absent GPIO (or clk) as if it were available.
>>>>
>>>> Hm, I've just looked at these and must note that they match 1:1 with
>>>> platform_get_irq_optional(). Unfortunately, we can't however behave the
>>>> same way in request_irq() -- because it has to support IRQ0 for the sake
>>>> of i8253 drivers in arch/...
>>>
>>> Let me reformulate your statement to the IMHO equivalent:
>>>
>>> If you set aside the differences between
>>> platform_get_irq_optional() and gpiod_get_optional(),
>>
>> Sorry, I should make it clear this is actually the diff between a would-be
>> platform_get_irq_optional() after my patch, not the current code...
>
> The similarity is that with your patch both gpiod_get_optional() and
> platform_get_irq_optional() return NULL and 0 on not-found. The relevant
> difference however is that for a gpiod NULL is a dummy value, while for
> irqs it's not. So the similarity is only syntactically, but not
> semantically.
>
>>> platform_get_irq_optional() is like gpiod_get_optional().
>>>
>>> The introduction of gpiod_get_optional() made it possible to simplify
>>> the following code:
>>>
>>> reset_gpio = gpiod_get(...)
>>> if IS_ERR(reset_gpio):
>>> error = PTR_ERR(reset_gpio)
>>> if error != -ENDEV:
>>
>> ENODEV?
>
> Yes, typo.
>
>>> return error
>>> else:
>>> gpiod_set_direction(reset_gpiod, INACTIVE)
>>>
>>> to
>>>
>>> reset_gpio = gpiod_get_optional(....)
>>> if IS_ERR(reset_gpio):
>>> return reset_gpio
>>> gpiod_set_direction(reset_gpiod, INACTIVE)
>>>
>>> and I never need to actually know if the reset_gpio actually exists.
>>> Either the line is put into its inactive state, or it doesn't exist and
>>> then gpiod_set_direction is a noop. For a regulator or a clk this works
>>> in a similar way.
>>>
>>> However for an interupt this cannot work. You will always have to check
>>> if the irq is actually there or not because if it's not you cannot just
>>> ignore that. So there is no benefit of an optional irq.
>>>
>>> Leaving error message reporting aside, the introduction of
>>> platform_get_irq_optional() allows to change
>>>
>>> irq = platform_get_irq(...);
>>> if (irq < 0 && irq != -ENXIO) {
>>> return irq;
>>> } else if (irq >= 0) {
>>
>> Rather (irq > 0) actually, IRQ0 is considered invalid (but still returned).
>
> This is a topic I don't feel strong for, so I'm sloppy here. If changing
> this is all that is needed to convince you of my point ...

See below. :-)

>>> ... setup irq operation ...
>>> } else { /* irq == -ENXIO */
>>> ... setup polling ...
>>> }
>>>
>>> to
>>>
>>> irq = platform_get_irq_optional(...);
>>> if (irq < 0 && irq != -ENXIO) {
>>> return irq;
>>> } else if (irq >= 0) {
>>> ... setup irq operation ...
>>> } else { /* irq == -ENXIO */
>>> ... setup polling ...
>>> }
>>>
>>> which isn't a win. When changing the return value as you suggest, it can
>>> be changed instead to:
>>>
>>> irq = platform_get_irq_optional(...);
>>> if (irq < 0) {
>>> return irq;
>>> } else if (irq > 0) {
>>> ... setup irq operation ...
>>> } else { /* irq == 0 */
>>> ... setup polling ...
>>> }
>>>
>>> which is a tad nicer. If that is your goal however I ask you to also
>>> change the semantic of platform_get_irq() to return 0 on "not found".
>>
>> Well, I'm not totally opposed to that... but would there be a considerable win?

> Well, compared to your suggestion of making platform_get_irq_optional()
> return 0 on "not-found" the considerable win would be that
> platform_get_irq_optional() and platform_get_irq() are not different

They would really be the same function if we do that. But...

> just because platform_get_irq() is to hard to change.

It's not just that, of course. If you make platform_get_irq() return 0
ISO -ENXIO, you'd have to add the handling of that 0 to all the callers,
and that won't be as simple as:

if (irq < 0)
return irq;

since we can't just propagate 0 upstream, we'd have to return something like
-ENXIO (or whatever error we see fit). Does that really scale well?

>> Anyway, we should 1st stop returning 0 for "valid" IRQs -- this is done by my patch
>> the discussed patch series are atop of.
>>
>>> Note the win is considerably less compared to gpiod_get_optional vs
>>
>> If there's any at all... We'd basically have to touch /all/ platform_get_irq()
>> calls (and get an even larger CC list ;-)).
>
> You got me wrong here. I meant that even if you change both
> platform_get_irq() and platform_get_irq_optional() to return 0 on
> "not-found", the win is small compared to the benefit of having both

There's no win at all, it seems.

> clk_get() and clk_get_optional().
>
>>> gpiod_get however. And then it still lacks the semantic of a dummy irq
>>> which IMHO forfeits the right to call it ..._optional().
>>
>> Not quite grasping it... Why e.g. clk_get() doesn't return 0 for a not found clock?
>
> Because NULL is not an error value for clk and when calling clk_get()
> you want a failure when the clk you asked for isn't available.
>
> Sure you could do the following in a case where you want to insist the
> clk to be actually available:
>
> clk = clk_get_optional(...)
> if (IS_ERR_OR_NULL(clk)) {
> err = PTR_ERR(clk) || -ENODEV;
> return dev_err_probe(dev, err, ....);
> }
>
> but this is more ugly than
>
> clk = clk_get(...)
> if (IS_ERR(clk)) {
> err = PTR_ERR(clk);
> return dev_err_probe(dev, err, ....);
> }
>
> Additionally the first usage would hard-code in the drivers that NULL is
> the dummy value which you might want to consider a layer violation.

Unfortunately, we don't have a single layer in case of IRQs... There's
no platform_request_irq() (yet? :-)).

> You have to understand that for clk (and regulator and gpiod) NULL is a
> valid descriptor that can actually be used, it just has no effect. So
> this is a convenience value for the case "If the clk/regulator/gpiod in
> question isn't available, there is nothing to do". This is what makes
> clk_get_optional() and the others really useful and justifies their
> existence. This doesn't apply to platform_get_irq_optional().

I do understand that. However, IRQs are a different beast with their
own justifications...

> So clk_get() is sane and sensible for cases where you need the clk to be
> there. It doesn't emit an error message, because the caller knows better
> if it's worth an error message and in some cases the caller can also
> emit a better error message than clk_get() itself.

I haven't been thinking about the IRQ error messages at all (yet?)...
And when I start thinking about it, it doesn't seem that bad, perhaps
even saves a lot of the .rodata section... :-)

> clk_get_optional() is sane and sensible for cases where the clk might be
> absent and it helps you because you don't have to differentiate between
> "not found" and "there is an actual resource".
>
> The reason for platform_get_irq_optional()'s existence is just that
> platform_get_irq() emits an error message which is wrong or suboptimal

I think you are very wrong here. The real reason is to simplify the
callers.

> in some cases (and IMHO is platform_get_irq() root fault). It doesn't
> simplify handling the "not found" case.

Oh, it does... you don't have to special-case 0 when handling its result.
In my book, it's a major simplification.

> So let's not pretend by the
> choice of function names that there is a similarity between clk_get() +
> clk_get_optional() and platform_get_irq() + platform_get_irq_optional().

OK, no similarity. But that's well justified.

> And as you cannot change platform_get_irq_optional() to return a working
> dummy value, IMHO the only sane way out is renaming it.

Your rename really focused on the wrong aspect of the function, I think...

> Best regards
> Uwe

MBR, Sergey

2022-01-16 16:23:08

by Sergey Shtylyov

[permalink] [raw]

Subject: Re: [PATCH] driver core: platform: Rename platform_get_irq_optional() to platform_get_irq_silent()

On 1/15/22 6:21 PM, Uwe Kleine-K?nig wrote:

[...]
>>>>>> The subsystems regulator, clk and gpio have the concept of a dummy
>>>>>> resource. For regulator, clk and gpio there is a semantic difference
>>>>>> between the regular _get() function and the _get_optional() variant.
>>>>>> (One might return the dummy resource, the other won't. Unfortunately
>>>>>> which one implements which isn't the same for these three.) The
>>>>>> difference between platform_get_irq() and platform_get_irq_optional() is
>>>>>> only that the former might emit an error message and the later won't.
>>>>
>>>> This is only a current difference but I'm still going to return 0 ISO
>>>> -ENXIO from latform_get_irq_optional(), no way I'd leave that -ENXIO there
>>>> alone... :-)
>>>
>>> This would address a bit of the critic in my commit log. But as 0 isn't
>>> a dummy value like the dummy values that exist for clk, gpiod and
>>> regulator I still think that the naming is a bad idea because it's not
>>> in the spirit of the other *_get_optional functions.
>>>
>>> Seeing you say that -ENXIO is a bad return value for
>>> platform_get_irq_optional() and 0 should be used instead, I wonder why
>>> not changing platform_get_irq() to return 0 instead of -ENXIO, too.
>>> This question is for now only about a sensible semantic. That actually
>>> changing platform_get_irq() is probably harder than changing
>>> platform_get_irq_optional() is a different story.
>>>
>>> If only platform_get_irq_optional() is changed and given that the
>>> callers have to do something like:
>>>
>>> if (this_irq_exists()):
>>> ... (e.g. request_irq)
>>> else:
>>> ... (e.g. setup polling)
>>>
>>> I really think it's a bad idea that this_irq_exists() has to be
>>> different for platform_get_irq() vs. platform_get_irq_optional().
>>
>> For platform_get_irq(), the IRQ being absent is an error condition,
>> hence it should return an error code.
>> For platform_get_irq_optional(), the IRQ being absent is not an error
>> condition, hence it should not return an error code, and 0 is OK.
>
> Please show a few examples how this simplifies the code. If it's only

As for platform_get_irq(), returning -ENXIO simplifies things a lot: you don't
have to check for 0 at every freaking call site and have s/th like (every
time!):

irq = platform_get_irq();
if (irq <= 0)
return irq ?: -ENXIO; // any error code you choose

instead of just:

irq = platform_get_irq();
if (irq < 0)
return irq;

This scales better in my book.

> that a driver has to check for == 0 instead of == -ENXIO, than that's
> not a good enough motivation to make platform_get_irq_optional()
> different to platform_get_irq().

Again, it scales better... good motivation in my eyes.

> Best regards
> Uwe

MBR, Sergey

2022-01-17 08:01:59

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional (summary)

On Sat, Jan 15, 2022 at 07:36:43PM +0100, Uwe Kleine-K?nig wrote:
> A possible compromise: We can have both. We rename
> platform_get_irq_optional() to platform_get_irq_silent() (or
> platform_get_irq_silently() if this is preferred) and once all users are
> are changed (which can be done mechanically), we reintroduce a
> platform_get_irq_optional() with Sergey's suggested semantic (i.e.
> return 0 on not-found, no error message printking).

Please do not do that as anyone trying to forward-port an old driver
will miss the abi change of functionality and get confused. Make
build-breaking changes, if the way a function currently works is
changed in order to give people a chance.

thanks,

greg k-h

2022-01-17 09:40:55

by Sergey Shtylyov

[permalink] [raw]

Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional

Hello!

On 1/14/22 11:22 PM, Uwe Kleine-K?nig wrote:

>>>>>>> To me it sounds much more logical for the driver to check if an
>>>>>>> optional irq is non-zero (available) or zero (not available), than to
>>>>>>> sprinkle around checks for -ENXIO. In addition, you have to remember
>>>>>>> that this one returns -ENXIO, while other APIs use -ENOENT or -ENOSYS
>>>>>>> (or some other error code) to indicate absence. I thought not having
>>>>>>> to care about the actual error code was the main reason behind the
>>>>>>> introduction of the *_optional() APIs.
>>>>>
>>>>>> No, the main benefit of gpiod_get_optional() (and clk_get_optional()) is
>>>>>> that you can handle an absent GPIO (or clk) as if it were available.
>>>>
>>>> Hm, I've just looked at these and must note that they match 1:1 with
>>>> platform_get_irq_optional(). Unfortunately, we can't however behave the
>>>> same way in request_irq() -- because it has to support IRQ0 for the sake
>>>> of i8253 drivers in arch/...
>>>
>>> Let me reformulate your statement to the IMHO equivalent:
>>>
>>> If you set aside the differences between
>>> platform_get_irq_optional() and gpiod_get_optional(),
>>
>> Sorry, I should make it clear this is actually the diff between a would-be
>> platform_get_irq_optional() after my patch, not the current code...
>
> The similarity is that with your patch both gpiod_get_optional() and
> platform_get_irq_optional() return NULL and 0 on not-found. The relevant
> difference however is that for a gpiod NULL is a dummy value, while for
> irqs it's not. So the similarity is only syntactically, but not
> semantically.

I have noting to say here, rather than optional IRQ could well have a different
meaning than for clk/gpio/etc.

[...]
>>> However for an interupt this cannot work. You will always have to check
>>> if the irq is actually there or not because if it's not you cannot just
>>> ignore that. So there is no benefit of an optional irq.
>>>
>>> Leaving error message reporting aside, the introduction of
>>> platform_get_irq_optional() allows to change
>>>
>>> irq = platform_get_irq(...);
>>> if (irq < 0 && irq != -ENXIO) {
>>> return irq;
>>> } else if (irq >= 0) {
>>
>> Rather (irq > 0) actually, IRQ0 is considered invalid (but still returned).
>
> This is a topic I don't feel strong for, so I'm sloppy here. If changing
> this is all that is needed to convince you of my point ...

Note that we should absolutely (and first of all) stop returning 0 from platform_get_irq()
on a "real" IRQ0. Handling that "still good" zero absolutely doesn't scale e.g. for the subsystems
(like libata) which take 0 as an indication that the polling mode should be used... We can't afford
to be sloppy here. ;-)

[...]

> Best regards
> Uwe

MBR, Sergey

2022-01-17 17:01:55

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional

On Sat, Jan 15, 2022 at 9:22 PM Sergey Shtylyov <[email protected]> wrote:
> On 1/14/22 11:22 PM, Uwe Kleine-König wrote:
> > You have to understand that for clk (and regulator and gpiod) NULL is a
> > valid descriptor that can actually be used, it just has no effect. So
> > this is a convenience value for the case "If the clk/regulator/gpiod in
> > question isn't available, there is nothing to do". This is what makes
> > clk_get_optional() and the others really useful and justifies their
> > existence. This doesn't apply to platform_get_irq_optional().
>
> I do understand that. However, IRQs are a different beast with their
> own justifications...

> > clk_get_optional() is sane and sensible for cases where the clk might be
> > absent and it helps you because you don't have to differentiate between
> > "not found" and "there is an actual resource".
> >
> > The reason for platform_get_irq_optional()'s existence is just that
> > platform_get_irq() emits an error message which is wrong or suboptimal
>
> I think you are very wrong here. The real reason is to simplify the
> callers.

Indeed.

Even for clocks, you cannot assume that you can always blindly use
the returned dummy (actually a NULL pointer) to call into the clk
API. While this works fine for simple use cases, where you just
want to enable/disable an optional clock (clk_prepare_enable() and
clk_disable_unprepare()), it does not work for more complex use cases.

Consider a device with multiple clock inputs, some of them optional,
where the device driver has to find, select and configure a suitable
clock to operate at a certain clock frequency. The driver can call
clk_get_rate(NULL) fine, but will always receive a zero rate, so it
has to check for this (regardless of this being a dummy clock or not,
because this could be an unpopulated clock crystal, which would be
described in DT as a (present) fixed-rate clock with clock-frequency
= <0>).
For configuring the clock rate, the driver does need to check
explicitly for the presence of a dummy clock, as clk_set_rate(NULL,
rate) returns 0 ("success"), while obviously it didn't do anything,
and thus configuring the device to use that clock would cause breakage.

You can check if the clock is a real clock or a dummy using
"if (clk) ...".
And you'd use the same pattern with platform_irq_get_optional() if it
would return 0 if the IRQ was not found: "if (irq) ...".

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2022-01-17 17:02:41

by Uwe Kleine-König

[permalink] [raw]

Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional

Hello,

On Sun, Jan 16, 2022 at 09:15:20PM +0300, Sergey Shtylyov wrote:
> On 1/14/22 11:22 PM, Uwe Kleine-K?nig wrote:
>
> >>>>>>> To me it sounds much more logical for the driver to check if an
> >>>>>>> optional irq is non-zero (available) or zero (not available), than to
> >>>>>>> sprinkle around checks for -ENXIO. In addition, you have to remember
> >>>>>>> that this one returns -ENXIO, while other APIs use -ENOENT or -ENOSYS
> >>>>>>> (or some other error code) to indicate absence. I thought not having
> >>>>>>> to care about the actual error code was the main reason behind the
> >>>>>>> introduction of the *_optional() APIs.
> >>>>>
> >>>>>> No, the main benefit of gpiod_get_optional() (and clk_get_optional()) is
> >>>>>> that you can handle an absent GPIO (or clk) as if it were available.
> >>>>
> >>>> Hm, I've just looked at these and must note that they match 1:1 with
> >>>> platform_get_irq_optional(). Unfortunately, we can't however behave the
> >>>> same way in request_irq() -- because it has to support IRQ0 for the sake
> >>>> of i8253 drivers in arch/...
> >>>
> >>> Let me reformulate your statement to the IMHO equivalent:
> >>>
> >>> If you set aside the differences between
> >>> platform_get_irq_optional() and gpiod_get_optional(),
> >>
> >> Sorry, I should make it clear this is actually the diff between a would-be
> >> platform_get_irq_optional() after my patch, not the current code...
> >
> > The similarity is that with your patch both gpiod_get_optional() and
> > platform_get_irq_optional() return NULL and 0 on not-found. The relevant
> > difference however is that for a gpiod NULL is a dummy value, while for
> > irqs it's not. So the similarity is only syntactically, but not
> > semantically.
>
> I have noting to say here, rather than optional IRQ could well have a different
> meaning than for clk/gpio/etc.
>
> [...]
> >>> However for an interupt this cannot work. You will always have to check
> >>> if the irq is actually there or not because if it's not you cannot just
> >>> ignore that. So there is no benefit of an optional irq.
> >>>
> >>> Leaving error message reporting aside, the introduction of
> >>> platform_get_irq_optional() allows to change
> >>>
> >>> irq = platform_get_irq(...);
> >>> if (irq < 0 && irq != -ENXIO) {
> >>> return irq;
> >>> } else if (irq >= 0) {
> >>
> >> Rather (irq > 0) actually, IRQ0 is considered invalid (but still returned).
> >
> > This is a topic I don't feel strong for, so I'm sloppy here. If changing
> > this is all that is needed to convince you of my point ...
>
> Note that we should absolutely (and first of all) stop returning 0 from platform_get_irq()
> on a "real" IRQ0. Handling that "still good" zero absolutely doesn't scale e.g. for the subsystems
> (like libata) which take 0 as an indication that the polling mode should be used... We can't afford
> to be sloppy here. ;-)

Then maybe do that really first? I didn't recheck, but is this what the
driver changes in your patch is about?

After some more thoughts I wonder if your focus isn't to align
platform_get_irq_optional to (clk|gpiod|regulator)_get_optional, but to
simplify return code checking. Because with your change we have:

- < 0 -> error
- == 0 -> no irq
- > 0 -> irq

For my part I'd say this doesn't justify the change, but at least I
could better life with the reasoning. If you start at:

irq = platform_get_irq_optional(...)
if (irq < 0 && irq != -ENXIO)
return irq
else if (irq > 0)
setup_irq(irq);
else
setup_polling()

I'd change that to

irq = platform_get_irq_optional(...)
if (irq > 0) /* or >= 0 ? */
setup_irq(irq)
else if (irq == -ENXIO)
setup_polling()
else
return irq

This still has to mention -ENXIO, but this is ok and checking for 0 just
hardcodes a different return value.

Anyhow, I think if you still want to change platform_get_irq_optional
you should add a few patches converting some drivers which demonstrates
the improvement for the callers.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |

Attachments:

(No filename) (4.18 kB)
signature.asc (499.00 B)
Download all attachments

2022-01-17 17:04:08

On Mon, Jan 17, 2022 at 11:35:52AM +0100, Geert Uytterhoeven wrote:
> Hi Uwe,
>
> On Mon, Jan 17, 2022 at 10:24 AM Uwe Kleine-K?nig
> <[email protected]> wrote:
> > On Mon, Jan 17, 2022 at 09:41:42AM +0100, Geert Uytterhoeven wrote:
> > > On Sat, Jan 15, 2022 at 9:22 PM Sergey Shtylyov <[email protected]> wrote:
> > > > On 1/14/22 11:22 PM, Uwe Kleine-K?nig wrote:
> > > > > You have to understand that for clk (and regulator and gpiod) NULL is a
> > > > > valid descriptor that can actually be used, it just has no effect. So
> > > > > this is a convenience value for the case "If the clk/regulator/gpiod in
> > > > > question isn't available, there is nothing to do". This is what makes
> > > > > clk_get_optional() and the others really useful and justifies their
> > > > > existence. This doesn't apply to platform_get_irq_optional().
> > > >
> > > > I do understand that. However, IRQs are a different beast with their
> > > > own justifications...
> > >
> > > > > clk_get_optional() is sane and sensible for cases where the clk might be
> > > > > absent and it helps you because you don't have to differentiate between
> > > > > "not found" and "there is an actual resource".
> > > > >
> > > > > The reason for platform_get_irq_optional()'s existence is just that
> > > > > platform_get_irq() emits an error message which is wrong or suboptimal
> > > >
> > > > I think you are very wrong here. The real reason is to simplify the
> > > > callers.
> > >
> > > Indeed.
> >
> > The commit that introduced platform_get_irq_optional() said:
> >
> > Introduce a new platform_get_irq_optional() that works much like
> > platform_get_irq() but does not output an error on failure to
> > find the interrupt.
> >
> > So the author of 8973ea47901c81a1912bd05f1577bed9b5b52506 failed to
> > mention the real reason? Or look at
> > 31a8d8fa84c51d3ab00bf059158d5de6178cf890:
> >
> > [...] use platform_get_irq_optional() to get second/third IRQ
> > which are optional to avoid below error message during probe:
> > [...]
> >
> > Look through the output of
> >
> > git log -Splatform_get_irq_optional
> >
> > to find several more of these.
>
> Commit 8973ea47901c81a1 ("driver core: platform: Introduce
> platform_get_irq_optional()") and the various fixups fixed the ugly
> printing of error messages that were not applicable.
> In hindsight, probably commit 7723f4c5ecdb8d83 ("driver core:
> platform: Add an error message to platform_get_irq*()") should have
> been reverted instead, until a platform_get_irq_optional() with proper
> semantics was introduced.

ack.

> But as we were all in a hurry to kill the non-applicable error
> message, we went for the quick and dirty fix.
>
> > Also I fail to see how a caller of (today's) platform_get_irq_optional()
> > is simpler than a caller of platform_get_irq() given that there is no
> > semantic difference between the two. Please show me a single
> > conversion from platform_get_irq to platform_get_irq_optional that
> > yielded a simplification.
>
> That's exactly why we want to change the latter to return 0 ;-)

OK. So you agree to my statement "The reason for
platform_get_irq_optional()'s existence is just that platform_get_irq()
emits an error message [...]". Actually you don't want to oppose but
say: It's unfortunate that the silent variant of platform_get_irq() took
the obvious name of a function that could have an improved return code
semantic.

So my suggestion to rename todays platform_get_irq_optional() to
platform_get_irq_silently() and then introducing
platform_get_irq_optional() with your suggested semantic seems
intriguing and straigt forward to me.

Another thought: platform_get_irq emits an error message for all
problems. Wouldn't it be consistent to let platform_get_irq_optional()
emit an error message for all problems but "not found"?
Alternatively remove the error printk from platform_get_irq().

> > So you need some more effort to convince me of your POV.
> >
> > > Even for clocks, you cannot assume that you can always blindly use
> > > the returned dummy (actually a NULL pointer) to call into the clk
> > > API. While this works fine for simple use cases, where you just
> > > want to enable/disable an optional clock (clk_prepare_enable() and
> > > clk_disable_unprepare()), it does not work for more complex use cases.
> >
> > Agreed. But for clks and gpiods and regulators the simple case is quite
> > usual. For irqs it isn't.
>
> It is for devices that can have either separate interrupts, or a single
> multiplexed interrupt.
>
> The logic in e.g. drivers/tty/serial/sh-sci.c and
> drivers/spi/spi-rspi.c could be simplified and improved (currently
> it doesn't handle deferred probe) if platform_get_irq_optional()
> would return 0 instead of -ENXIO.

Looking at sh-sci.c the irq handling logic could be improved even
without a changed platform_get_irq_optional():

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 968967d722d4..c7dc9fb84844 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -2873,11 +2873,13 @@ static int sci_init_single(struct platform_device *dev,
* interrupt ID numbers, or muxed together with another interrupt.
*/
if (sci_port->irqs[0] < 0)
- return -ENXIO;
+ return sci_port->irqs[0];

- if (sci_port->irqs[1] < 0)
+ if (sci_port->irqs[1] == -ENXIO)
for (i = 1; i < ARRAY_SIZE(sci_port->irqs); i++)
sci_port->irqs[i] = sci_port->irqs[0];
+ else if (sci_port->irqs[1] < 0)
+ return sci_port->irqs[1];

sci_port->params = sci_probe_regmap(p);
if (unlikely(sci_port->params == NULL))

And then the code flow is actively irritating. sci_init_single() copies
irqs[0] to all other irqs[i] and then sci_request_irq() loops over the
already requested irqs and checks for duplicates. A single place that
identifies the exact set of required irqs would already help a lot.

Also for spi-rspi.c I don't see how platform_get_irq_byname_optional()
returning 0 instead of -ENXIO would help. Please talk in patches.

Preferably first simplify in-driver logic to make the conversion to the
new platform_get_irq_optional() actually reviewable.

> > And if you cannot blindly use the dummy, then you're not the targetted
> > caller of *_get_optional() and should better use *_get() and handle
> > -ENODEV explicitly.
>
> No, because the janitors tend to consolidate error message handling,
> by moving the printing up, inside the *_get() methods. That's exactly
> what happened here.

This is in my eyes the root cause of the issues at hand. Moving the
error message handling into a get function is only right for most of the
callers. So the more conservative approach would be to introduce a noisy
variant of the get function and convert all users that benefit
separately while the unreviewed callers and those that don't want an
error message can happily continue to use the silent variant.

> So there are three reasons: because the absence of an optional IRQ
> is not an error, and thus that should not cause (a) an error code
> to be returned, and (b) an error message to be printed, and (c)
> because it can simplify the logic in device drivers.

I don't agree to (a). If the value signaling not-found is -ENXIO or 0
(or -ENODEV) doesn't matter much. I wouldn't deviate from the return
code semantics of platform_get_irq() just for having to check against 0
instead of -ENXIO. Zero is then just another magic value.
(c) still has to be proven, see above.

> Commit 8973ea47901c81a1 ("driver core: platform: Introduce
> platform_get_irq_optional()") fixed (b), but didn't address (a) and
> (c).

Yes, it fixed (b) and picked a bad name for that.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |

Attachments:

(No filename) (7.88 kB)
signature.asc (499.00 B)
Download all attachments

2022-01-17 17:16:24

by Sergey Shtylyov

[permalink] [raw]

Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional

On 1/10/22 10:54 PM, Sergey Shtylyov wrote:

> This patch is based on the former Andy Shevchenko's patch:
>
> https://lore.kernel.org/lkml/[email protected]/
>
> Currently platform_get_irq_optional() returns an error code even if IRQ
> resource simply has not been found. It prevents the callers from being
> error code agnostic in their error handling:
>
> ret = platform_get_irq_optional(...);
> if (ret < 0 && ret != -ENXIO)
> return ret; // respect deferred probe
> if (ret > 0)
> ...we get an IRQ...
>
> All other *_optional() APIs seem to return 0 or NULL in case an optional
> resource is not available. Let's follow this good example, so that the
> callers would look like:
>
> ret = platform_get_irq_optional(...);
> if (ret < 0)
> return ret;
> if (ret > 0)
> ...we get an IRQ...
>
> Reported-by: Matthias Schiffer <[email protected]>
> Signed-off-by: Sergey Shtylyov <[email protected]>
[...]

Please don't merge this as yet, I'm going thru this patch once again
and have already found some sloppy code. :-/

> diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c
> index 7450904e330a..fdc63bfa5be4 100644
> --- a/drivers/char/ipmi/bt-bmc.c
> +++ b/drivers/char/ipmi/bt-bmc.c
> @@ -382,12 +382,14 @@ static int bt_bmc_config_irq(struct bt_bmc *bt_bmc,
> bt_bmc->irq = platform_get_irq_optional(pdev, 0);
> if (bt_bmc->irq < 0)
> return bt_bmc->irq;
> + if (!bt_bmc->irq)
> + return 0;

Hm, this is sloppy. Will recast and rebase to the -next branch.

>
> rc = devm_request_irq(dev, bt_bmc->irq, bt_bmc_irq, IRQF_SHARED,
> DEVICE_NAME, bt_bmc);
> if (rc < 0) {
> dev_warn(dev, "Unable to request IRQ %d\n", bt_bmc->irq);
> - bt_bmc->irq = rc;
> + bt_bmc->irq = 0;

This change isn't needed...

> return rc;
> }
>
[...]
> diff --git a/drivers/edac/xgene_edac.c b/drivers/edac/xgene_edac.c
> index 2ccd1db5e98f..0d1bdd27cd78 100644
> --- a/drivers/edac/xgene_edac.c
> +++ b/drivers/edac/xgene_edac.c
> @@ -1917,7 +1917,7 @@ static int xgene_edac_probe(struct platform_device *pdev)
>
> for (i = 0; i < 3; i++) {
> irq = platform_get_irq_optional(pdev, i);

Is *_optinal() even correct here?

> - if (irq < 0) {
> + if (irq <= 0) {
> dev_err(&pdev->dev, "No IRQ resource\n");
> rc = -EINVAL;
> goto out_err;
[...]
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index f75929783b94..ac222985efde 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -1521,7 +1521,7 @@ static irqreturn_t brcmnand_ctlrdy_irq(int irq, void *data)
>
> /* check if you need to piggy back on the ctrlrdy irq */
> if (ctrl->edu_pending) {
> - if (irq == ctrl->irq && ((int)ctrl->edu_irq >= 0))
> + if (irq == ctrl->irq && ((int)ctrl->edu_irq > 0))

Note to self: the cast to *int* isn't needed, the edu_irq field is *int* already...

[...]
> diff --git a/drivers/power/supply/mp2629_charger.c b/drivers/power/supply/mp2629_charger.c
> index bdf924b73e47..51289700a7ac 100644
> --- a/drivers/power/supply/mp2629_charger.c
> +++ b/drivers/power/supply/mp2629_charger.c
> @@ -581,9 +581,9 @@ static int mp2629_charger_probe(struct platform_device *pdev)
> platform_set_drvdata(pdev, charger);
>
> irq = platform_get_irq_optional(to_platform_device(dev->parent), 0);

Again, is *_optional() even correct here?

> - if (irq < 0) {
> + if (irq <= 0) {
> dev_err(dev, "get irq fail: %d\n", irq);
> - return irq;
> + return irq < 0 ? irq : -ENXIO;
> }
>
> for (i = 0; i < MP2629_MAX_FIELD; i++) {
[...]
> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> index 43eb25b167bc..776cfed4339c 100644
> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -430,7 +430,7 @@ static int rcar_gen3_thermal_request_irqs(struct rcar_gen3_thermal_priv *priv,
>
> for (i = 0; i < 2; i++) {
> irq = platform_get_irq_optional(pdev, i);
> - if (irq < 0)
> + if (irq <= 0)
> return irq;

Sloppy code again? We shouldn't return 0...

[...]
> diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
> index 68a1c87066d7..cd7494933563 100644
> --- a/drivers/vfio/platform/vfio_platform.c
> +++ b/drivers/vfio/platform/vfio_platform.c
> @@ -32,8 +32,12 @@ static struct resource *get_platform_resource(struct vfio_platform_device *vdev,
> static int get_platform_irq(struct vfio_platform_device *vdev, int i)
> {
> struct platform_device *pdev = (struct platform_device *) vdev->opaque;
> + int ret;
>
> - return platform_get_irq_optional(pdev, i);
> + ret = platform_get_irq_optional(pdev, i);
> + if (ret < 0)
> + return ret;
> + return ret > 0 ? ret : -ENXIO;

Could be expressed more concisely:

return ret ? : -ENXIO;

just like vfio_amba.c does it...

[...]

MBR, Sergey

2022-01-18 02:26:24

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional

Hi Uwe,

On Mon, Jan 17, 2022 at 12:49 PM Uwe Kleine-König
<[email protected]> wrote:
> On Mon, Jan 17, 2022 at 11:35:52AM +0100, Geert Uytterhoeven wrote:
> > On Mon, Jan 17, 2022 at 10:24 AM Uwe Kleine-König
> > <[email protected]> wrote:
> > > On Mon, Jan 17, 2022 at 09:41:42AM +0100, Geert Uytterhoeven wrote:
> > > > On Sat, Jan 15, 2022 at 9:22 PM Sergey Shtylyov <[email protected]> wrote:
> > > > > On 1/14/22 11:22 PM, Uwe Kleine-König wrote:
> > > > > > You have to understand that for clk (and regulator and gpiod) NULL is a
> > > > > > valid descriptor that can actually be used, it just has no effect. So
> > > > > > this is a convenience value for the case "If the clk/regulator/gpiod in
> > > > > > question isn't available, there is nothing to do". This is what makes
> > > > > > clk_get_optional() and the others really useful and justifies their
> > > > > > existence. This doesn't apply to platform_get_irq_optional().
> > > > >
> > > > > I do understand that. However, IRQs are a different beast with their
> > > > > own justifications...
> > > >
> > > > > > clk_get_optional() is sane and sensible for cases where the clk might be
> > > > > > absent and it helps you because you don't have to differentiate between
> > > > > > "not found" and "there is an actual resource".
> > > > > >
> > > > > > The reason for platform_get_irq_optional()'s existence is just that
> > > > > > platform_get_irq() emits an error message which is wrong or suboptimal
> > > > >
> > > > > I think you are very wrong here. The real reason is to simplify the
> > > > > callers.
> > > >
> > > > Indeed.
> > >
> > > The commit that introduced platform_get_irq_optional() said:
> > >
> > > Introduce a new platform_get_irq_optional() that works much like
> > > platform_get_irq() but does not output an error on failure to
> > > find the interrupt.
> > >
> > > So the author of 8973ea47901c81a1912bd05f1577bed9b5b52506 failed to
> > > mention the real reason? Or look at
> > > 31a8d8fa84c51d3ab00bf059158d5de6178cf890:
> > >
> > > [...] use platform_get_irq_optional() to get second/third IRQ
> > > which are optional to avoid below error message during probe:
> > > [...]
> > >
> > > Look through the output of
> > >
> > > git log -Splatform_get_irq_optional
> > >
> > > to find several more of these.
> >
> > Commit 8973ea47901c81a1 ("driver core: platform: Introduce
> > platform_get_irq_optional()") and the various fixups fixed the ugly
> > printing of error messages that were not applicable.
> > In hindsight, probably commit 7723f4c5ecdb8d83 ("driver core:
> > platform: Add an error message to platform_get_irq*()") should have
> > been reverted instead, until a platform_get_irq_optional() with proper
> > semantics was introduced.
>
> ack.
>
> > But as we were all in a hurry to kill the non-applicable error
> > message, we went for the quick and dirty fix.
> >
> > > Also I fail to see how a caller of (today's) platform_get_irq_optional()
> > > is simpler than a caller of platform_get_irq() given that there is no
> > > semantic difference between the two. Please show me a single
> > > conversion from platform_get_irq to platform_get_irq_optional that
> > > yielded a simplification.
> >
> > That's exactly why we want to change the latter to return 0 ;-)
>
> OK. So you agree to my statement "The reason for
> platform_get_irq_optional()'s existence is just that platform_get_irq()
> emits an error message [...]". Actually you don't want to oppose but
> say: It's unfortunate that the silent variant of platform_get_irq() took
> the obvious name of a function that could have an improved return code
> semantic.
>
> So my suggestion to rename todays platform_get_irq_optional() to
> platform_get_irq_silently() and then introducing
> platform_get_irq_optional() with your suggested semantic seems
> intriguing and straigt forward to me.

I don't really see the point of needing platform_get_irq_silently(),
unless as an intermediary step, where it's going to be removed again
once the conversion has completed.
Still, the rename would touch all users at once anyway.

> Another thought: platform_get_irq emits an error message for all
> problems. Wouldn't it be consistent to let platform_get_irq_optional()
> emit an error message for all problems but "not found"?
> Alternatively remove the error printk from platform_get_irq().

Yes, all problems but not found are real errors.

> > > So you need some more effort to convince me of your POV.
> > >
> > > > Even for clocks, you cannot assume that you can always blindly use
> > > > the returned dummy (actually a NULL pointer) to call into the clk
> > > > API. While this works fine for simple use cases, where you just
> > > > want to enable/disable an optional clock (clk_prepare_enable() and
> > > > clk_disable_unprepare()), it does not work for more complex use cases.
> > >
> > > Agreed. But for clks and gpiods and regulators the simple case is quite
> > > usual. For irqs it isn't.
> >
> > It is for devices that can have either separate interrupts, or a single
> > multiplexed interrupt.
> >
> > The logic in e.g. drivers/tty/serial/sh-sci.c and
> > drivers/spi/spi-rspi.c could be simplified and improved (currently
> > it doesn't handle deferred probe) if platform_get_irq_optional()
> > would return 0 instead of -ENXIO.
>
> Looking at sh-sci.c the irq handling logic could be improved even
> without a changed platform_get_irq_optional():
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 968967d722d4..c7dc9fb84844 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -2873,11 +2873,13 @@ static int sci_init_single(struct platform_device *dev,
> * interrupt ID numbers, or muxed together with another interrupt.
> */
> if (sci_port->irqs[0] < 0)
> - return -ENXIO;
> + return sci_port->irqs[0];
>
> - if (sci_port->irqs[1] < 0)
> + if (sci_port->irqs[1] == -ENXIO)
> for (i = 1; i < ARRAY_SIZE(sci_port->irqs); i++)
> sci_port->irqs[i] = sci_port->irqs[0];
> + else if (sci_port->irqs[1] < 0)
> + return sci_port->irqs[1];
>
> sci_port->params = sci_probe_regmap(p);
> if (unlikely(sci_port->params == NULL))
>
> And then the code flow is actively irritating. sci_init_single() copies
> irqs[0] to all other irqs[i] and then sci_request_irq() loops over the
> already requested irqs and checks for duplicates. A single place that
> identifies the exact set of required irqs would already help a lot.

Yeah, it's ugly and convoluted, like the wide set of hardware the
driver supports.

> Also for spi-rspi.c I don't see how platform_get_irq_byname_optional()
> returning 0 instead of -ENXIO would help. Please talk in patches.

--- a/drivers/spi/spi-rspi.c
+++ b/drivers/spi/spi-rspi.c
@@ -1420,17 +1420,25 @@ static int rspi_probe(struct platform_device *pdev)
ctlr->max_native_cs = rspi->ops->num_hw_ss;

ret = platform_get_irq_byname_optional(pdev, "rx");
- if (ret < 0) {
+ if (ret < 0)
+ goto error2;
+
+ if (!ret) {
ret = platform_get_irq_byname_optional(pdev, "mux");
- if (ret < 0)
+ if (!ret)
ret = platform_get_irq(pdev, 0);
+ if (ret < 0)
+ goto error2;
+
if (ret >= 0)
rspi->rx_irq = rspi->tx_irq = ret;
} else {
rspi->rx_irq = ret;
ret = platform_get_irq_byname(pdev, "tx");
- if (ret >= 0)
- rspi->tx_irq = ret;
+ if (ret < 0)
+ goto error2;
+
+ rspi->tx_irq = ret;
}

if (rspi->rx_irq == rspi->tx_irq) {

I like it when the "if (ret < ) ..." error handling is the first check to do.
With -ENXIO, it becomes more convoluted. and looks less nice (IMHO).

> Preferably first simplify in-driver logic to make the conversion to the
> new platform_get_irq_optional() actually reviewable.

So I have to choose between

if (ret < 0 && ret != -ENXIO)
return ret;

if (ret) {
...
}

and

if (ret == -ENXIO) {
...
} else if (ret < 0)
return ret;
}

with the final target being

if (ret < 0)
return ret;

if (ret) {
...
}

So the first option means the final change is smaller, but it looks less
nice than the second option (IMHO).
But the second option means more churn.

> > So there are three reasons: because the absence of an optional IRQ
> > is not an error, and thus that should not cause (a) an error code
> > to be returned, and (b) an error message to be printed, and (c)
> > because it can simplify the logic in device drivers.
>
> I don't agree to (a). If the value signaling not-found is -ENXIO or 0
> (or -ENODEV) doesn't matter much. I wouldn't deviate from the return
> code semantics of platform_get_irq() just for having to check against 0
> instead of -ENXIO. Zero is then just another magic value.

Zero is a natural magic value (also for pointers).
Errors are always negative.
Positive values are cookies (or pointers) associated with success.

> (c) still has to be proven, see above.
>
> > Commit 8973ea47901c81a1 ("driver core: platform: Introduce
> > platform_get_irq_optional()") fixed (b), but didn't address (a) and
> > (c).
>
> Yes, it fixed (b) and picked a bad name for that.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2022-01-18 02:53:42

by Andy Shevchenko

[permalink] [raw]

[permalink] [raw]

Subject: Re: [PATCH] driver core: platform: Rename platform_get_irq_optional() to platform_get_irq_silent()

On Tue, Jan 25, 2022 at 01:56:05PM +0100, Matthias Schiffer wrote:
> On Tue, 2022-01-25 at 09:25 +0100, Geert Uytterhoeven wrote:
> > On Mon, Jan 24, 2022 at 10:02 PM Sergey Shtylyov <[email protected]>
> > wrote:
> > > On 1/24/22 6:01 PM, Andy Shevchenko wrote:

...

> > > > > > 2. The vIRQ0 handling: a) WARN() followed by b) returned
> > > > > > value 0
> > > > >
> > > > > I'm happy with the vIRQ0 handling. Today platform_get_irq() and
> > > > > it's
> > > > > silent variant returns either a valid and usuable irq number or
> > > > > a
> > > > > negative error value. That's totally fine.
> > > >
> > > > It might return 0.
> > > > Actually it seems that the WARN() can only be issued in two
> > > > cases:
> > > > - SPARC with vIRQ0 in one of the array member
> > > > - fallback to ACPI for GPIO IRQ resource with index 0
> > >
> > > You have probably missed the recent discovery that
> > > arch/sh/boards/board-aps4*.c
> > > causes IRQ0 to be passed as a direct IRQ resource?
> >
> > So far no one reported seeing the big fat warning ;-)
>
> FWIW, we had a similar issue with an IRQ resource passed from the
> tqmx86 MFD driver do the GPIO driver, which we noticed due to this
> warning, and which was fixed
> in a946506c48f3bd09363c9d2b0a178e55733bcbb6
> and 9b87f43537acfa24b95c236beba0f45901356eb2.

No, it's not, unfortunately :-( You just band aided the warning issue, but the
root cause is the WARN() and possibility to see valid (v)IRQ0 in the resources.
See below.

> I believe these changes are what promted this whole discussion and led
> to my "Reported-by" on the patch?
>
> It is not entirely clear to me when IRQ 0 is valid and when it isn't,
> but the warning seems useful to me. Maybe it would make more sense to
> warn when such an IRQ resource is registered for a platform device, and
> not when it is looked up?
>
> My opinion is that it would be very confusing if there are any places
> in the kernel (on some platforms) where IRQ 0 is valid,

And those places are board files like yours :( They have to be fixed
eventually. Ideally by using IRQ domains. At least that's how it's
done elsewhere.

> but for
> platform_get_irq() it would suddenly mean "not found". Keeping a
> negative return value seems preferable to me for this reason.

IRQ 0 is valid, vIRQ0 (or read it as cookie) is not.

Now, the problem in your case is that you are talking about board files, while
ACPI and DT never gives resource with vIRQ0. For board files some (legacy) code
decides that it's fine to supply HW IRQ, while the de facto case is that
platform_get_resource() returns whatever is in the resource, while
platform_get_irq() should return a cookie.

> (An alternative, more involved idea would be to add 1 to all IRQ
> "cookies", so IRQ 0 would return 1, leaving 0 as a special value. I
> have absolutely no idea how big the API surface is that would need
> changes, and it is likely not worth the effort at all.)

This is what IRQ domains do, they start vIRQs from 1.

> > > > The bottom line here is the SPARC case. Anybody familiar with the
> > > > platform
> > > > can shed a light on this. If there is no such case, we may remove
> > > > warning
> > > > along with ret = 0 case from platfrom_get_irq().
> > >
> > > I'm afraid you're too fast here... :-)
> > > We'll have a really hard time if we continue to allow IRQ0 to be
> > > returned by
> > > platform_get_irq() -- we'll have oto fileter it out in the callers
> > > then...
> >
> > So far no one reported seeing the big fat warning?
> >
> > > > > > 3. The specific cookie for "IRQ not found, while no error
> > > > > > happened" case
> > > > >
> > > > > Not sure what you mean here. I have no problem that a situation
> > > > > I can
> > > > > cope with is called an error for the query function. I just do
> > > > > error
> > > > > handling and continue happily. So the part "while no error
> > > > > happened" is
> > > > > irrelevant to me.
> > > >
> > > > I meant that instead of using special error code, 0 is very much
> > > > good for
> > > > the cases when IRQ is not found. It allows to distinguish -ENXIO
> > > > from the
> > > > low layer from -ENXIO with this magic meaning.
> > >
> > > I don't see how -ENXIO can trickle from the lower layers,
> > > frankly...
> >
> > It might one day, leading to very hard to track bugs.
>
> As gregkh noted, changing the return value without also making the
> compile fail will be a huge PITA whenever driver patches are back- or
> forward-ported, as it would require subtle changes in error paths,
> which can easily slip through unnoticed, in particular with half-
> automated stable backports.

Let's not modify kernel at all then, because in many cases it is a PITA
for back- or forward-porting :-)

> Even if another return value like -ENODEV might be better aligned with
> ...regulator_get_optional() and similar functions, or we even find a
> way to make 0 usable for this, none of the proposed changes strike me
> as big enough a win to outweigh the churn caused by making such a
> change at all.

Yeah, let's continue to suffer from ugly interface and see more band aids
landing around...

--
With Best Regards,
Andy Shevchenko