| Message ID | 20220110195449.12448-2-s.shtylyov@omp.ru |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/2] platform: make platform_get_irq_optional() optional | expand |
Hello, 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/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/ > > 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. 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(). Best regards Uwe
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/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/ > > > > 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. > 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.
On Mon, Jan 10, 2022 at 09:10:14PM +0100, Uwe Kleine-König wrote: > Hello, > > 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/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/ > > > > 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. > 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 is a good idea. Andrew
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/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/ > > > > > > 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
Hi Andrew, On Mon, Jan 10, 2022 at 10:20 PM Andrew Lunn <andrew@lunn.ch> 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/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/ > > > > > > 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 -- geert@linux-m68k.org 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
On Wed, Jan 12, 2022 at 09:33:48AM +0100, Geert Uytterhoeven wrote: > Hi Andrew, > > On Mon, Jan 10, 2022 at 10:20 PM Andrew Lunn <andrew@lunn.ch> 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/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/ > > > > > > > > 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. Is this an ack for renaming platform_get_irq_optional() to platform_get_irq_silent()? And then a coccinelle or sparse or ... hook that catches people testing the return value against 0 would be great. Best regards Uwe
Hi Uwe, On Wed, Jan 12, 2022 at 9:51 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Wed, Jan 12, 2022 at 09:33:48AM +0100, Geert Uytterhoeven wrote: > > On Mon, Jan 10, 2022 at 10:20 PM Andrew Lunn <andrew@lunn.ch> 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/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/ > > > > > > > > > > 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. > > Is this an ack for renaming platform_get_irq_optional() to > platform_get_irq_silent()? No it isn't ;-) If an optional IRQ is not present, drivers either just ignore it (e.g. for devices that can have multiple interrupts or a single muxed IRQ), or they have to resort to polling. For the latter, fall-back handling is needed elsewhere in the driver. 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. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
On Wed, Jan 12, 2022 at 11:27:02AM +0100, Geert Uytterhoeven wrote: > On Wed, Jan 12, 2022 at 9:51 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Wed, Jan 12, 2022 at 09:33:48AM +0100, Geert Uytterhoeven wrote: > > > On Mon, Jan 10, 2022 at 10:20 PM Andrew Lunn <andrew@lunn.ch> 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/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/ > > > > > > > > > > > > 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. > > > > Is this an ack for renaming platform_get_irq_optional() to > > platform_get_irq_silent()? > > No it isn't ;-) > > If an optional IRQ is not present, drivers either just ignore it (e.g. > for devices that can have multiple interrupts or a single muxed IRQ), > or they have to resort to polling. For the latter, fall-back handling > is needed elsewhere in the driver. > 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. For the record, I'm on the same page with Geert.
> If an optional IRQ is not present, drivers either just ignore it (e.g. > for devices that can have multiple interrupts or a single muxed IRQ), > or they have to resort to polling. For the latter, fall-back handling > is needed elsewhere in the driver. > 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. The *_optional() functions return an error code if there has been a real error which should be reported up the call stack. This excludes whatever error code indicates the requested resource does not exist, which can be -ENODEV etc. If the device does not exist, a magic cookie is returned which appears to be a valid resources but in fact is not. So the users of these functions just need to check for an error code, and fail the probe if present. You seems to be suggesting in binary return value: non-zero (available) or zero (not available) This discards the error code when something goes wrong. That is useful information to have, so we should not be discarding it. IRQ don't currently have a magic cookie value. One option would be to add such a magic cookie to the subsystem. Otherwise, since 0 is invalid, return 0 to indicate the IRQ does not exist. The request for a script checking this then makes sense. However, i don't know how well coccinelle/sparse can track values across function calls. They probably can check for: ret = irq_get_optional() if (ret < 0) return ret; A missing if < 0 statement somewhere later is very likely to be an error. A comparison of <= 0 is also likely to be an error. A check for > 0 before calling any other IRQ functions would be good. I'm surprised such a check does not already existing in the IRQ API, but there are probably historical reasons for that. Andrew
On Wed, Jan 12, 2022 at 02:38:37PM +0100, Andrew Lunn wrote: > > If an optional IRQ is not present, drivers either just ignore it (e.g. > > for devices that can have multiple interrupts or a single muxed IRQ), > > or they have to resort to polling. For the latter, fall-back handling > > is needed elsewhere in the driver. > > 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. > > The *_optional() functions return an error code if there has been a > real error which should be reported up the call stack. This excludes > whatever error code indicates the requested resource does not exist, > which can be -ENODEV etc. If the device does not exist, a magic cookie > is returned which appears to be a valid resources but in fact is > not. So the users of these functions just need to check for an error > code, and fail the probe if present. > > You seems to be suggesting in binary return value: non-zero > (available) or zero (not available) No, what is suggested is to (besides the API changes): - do not treat ENXIO as something special in platform_get_irq*() - allow platform_get_irq*() to return other error codes > This discards the error code when something goes wrong. That is useful > information to have, so we should not be discarding it. > > IRQ don't currently have a magic cookie value. One option would be to > add such a magic cookie to the subsystem. Otherwise, since 0 is > invalid, return 0 to indicate the IRQ does not exist. > > The request for a script checking this then makes sense. However, i > don't know how well coccinelle/sparse can track values across function > calls. They probably can check for: > > ret = irq_get_optional() > if (ret < 0) > return ret; > > A missing if < 0 statement somewhere later is very likely to be an > error. A comparison of <= 0 is also likely to be an error. A check for > > 0 before calling any other IRQ functions would be good. I'm > surprised such a check does not already existing in the IRQ API, but > there are probably historical reasons for that. > > Andrew
Hi Andrew, On Wed, Jan 12, 2022 at 2:38 PM Andrew Lunn <andrew@lunn.ch> wrote: > > If an optional IRQ is not present, drivers either just ignore it (e.g. > > for devices that can have multiple interrupts or a single muxed IRQ), > > or they have to resort to polling. For the latter, fall-back handling > > is needed elsewhere in the driver. > > 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. > > The *_optional() functions return an error code if there has been a > real error which should be reported up the call stack. This excludes > whatever error code indicates the requested resource does not exist, > which can be -ENODEV etc. If the device does not exist, a magic cookie > is returned which appears to be a valid resources but in fact is > not. So the users of these functions just need to check for an error > code, and fail the probe if present. Agreed. Note that in most (all?) other cases, the return type is a pointer (e.g. to struct clk), and NULL is the magic cookie. > You seems to be suggesting in binary return value: non-zero > (available) or zero (not available) Only in case of success. In case of a real failure, an error code must be returned. > This discards the error code when something goes wrong. That is useful > information to have, so we should not be discarding it. No, the error code must be retained in case of failure. > IRQ don't currently have a magic cookie value. One option would be to > add such a magic cookie to the subsystem. Otherwise, since 0 is > invalid, return 0 to indicate the IRQ does not exist. Exactly. And using 0 means the similar code can be used as for other subsystems, where NULL would be returned. The only remaining difference is the "dummy cookie can be passed to other functions" behavior. Which is IMHO a valid difference, as unlike with e.g. clk_prepare_enable(), you do pass extra data to request_irq(), and sometimes you do need to handle the absence of the interrupt using e.g. polling. > The request for a script checking this then makes sense. However, i > don't know how well coccinelle/sparse can track values across function > calls. They probably can check for: > > ret = irq_get_optional() > if (ret < 0) > return ret; > > A missing if < 0 statement somewhere later is very likely to be an > error. A comparison of <= 0 is also likely to be an error. A check for > > 0 before calling any other IRQ functions would be good. I'm > surprised such a check does not already existing in the IRQ API, but > there are probably historical reasons for that. There are still a few platforms where IRQ 0 does exist. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
On Wed, Jan 12, 2022 at 2:55 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Andrew, > > On Wed, Jan 12, 2022 at 2:38 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > If an optional IRQ is not present, drivers either just ignore it (e.g. > > > for devices that can have multiple interrupts or a single muxed IRQ), > > > or they have to resort to polling. For the latter, fall-back handling > > > is needed elsewhere in the driver. > > > 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. > > > > The *_optional() functions return an error code if there has been a > > real error which should be reported up the call stack. This excludes > > whatever error code indicates the requested resource does not exist, > > which can be -ENODEV etc. If the device does not exist, a magic cookie > > is returned which appears to be a valid resources but in fact is > > not. So the users of these functions just need to check for an error > > code, and fail the probe if present. > > Agreed. > > Note that in most (all?) other cases, the return type is a pointer > (e.g. to struct clk), and NULL is the magic cookie. > > > You seems to be suggesting in binary return value: non-zero > > (available) or zero (not available) > > Only in case of success. In case of a real failure, an error code > must be returned. > > > This discards the error code when something goes wrong. That is useful > > information to have, so we should not be discarding it. > > No, the error code must be retained in case of failure. > > > IRQ don't currently have a magic cookie value. One option would be to > > add such a magic cookie to the subsystem. Otherwise, since 0 is > > invalid, return 0 to indicate the IRQ does not exist. > > Exactly. And using 0 means the similar code can be used as for other > subsystems, where NULL would be returned. > > The only remaining difference is the "dummy cookie can be passed > to other functions" behavior. Which is IMHO a valid difference, > as unlike with e.g. clk_prepare_enable(), you do pass extra data to > request_irq(), and sometimes you do need to handle the absence of > the interrupt using e.g. polling. > > > The request for a script checking this then makes sense. However, i > > don't know how well coccinelle/sparse can track values across function > > calls. They probably can check for: > > > > ret = irq_get_optional() > > if (ret < 0) > > return ret; > > > > A missing if < 0 statement somewhere later is very likely to be an > > error. A comparison of <= 0 is also likely to be an error. A check for > > > 0 before calling any other IRQ functions would be good. I'm > > surprised such a check does not already existing in the IRQ API, but > > there are probably historical reasons for that. > > There are still a few platforms where IRQ 0 does exist. Not just a few even. This happens on a reasonably recent x86 PC: rafael@gratch:~/work/linux-pm> head -2 /proc/interrupts CPU0 CPU1 CPU2 CPU3 CPU4 CPU5 0: 10 0 0 0 0 0 IR-IO-APIC 2-edge timer
On 1/12/22 5:41 PM, Rafael J. Wysocki wrote: [...] >>>> If an optional IRQ is not present, drivers either just ignore it (e.g. >>>> for devices that can have multiple interrupts or a single muxed IRQ), >>>> or they have to resort to polling. For the latter, fall-back handling >>>> is needed elsewhere in the driver. >>>> 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. >>> >>> The *_optional() functions return an error code if there has been a >>> real error which should be reported up the call stack. This excludes >>> whatever error code indicates the requested resource does not exist, >>> which can be -ENODEV etc. If the device does not exist, a magic cookie >>> is returned which appears to be a valid resources but in fact is >>> not. So the users of these functions just need to check for an error >>> code, and fail the probe if present. >> >> Agreed. >> >> Note that in most (all?) other cases, the return type is a pointer >> (e.g. to struct clk), and NULL is the magic cookie. >> >>> You seems to be suggesting in binary return value: non-zero >>> (available) or zero (not available) >> >> Only in case of success. In case of a real failure, an error code >> must be returned. >> >>> This discards the error code when something goes wrong. That is useful >>> information to have, so we should not be discarding it. >> >> No, the error code must be retained in case of failure. >> >>> IRQ don't currently have a magic cookie value. One option would be to >>> add such a magic cookie to the subsystem. Otherwise, since 0 is >>> invalid, return 0 to indicate the IRQ does not exist. >> >> Exactly. And using 0 means the similar code can be used as for other >> subsystems, where NULL would be returned. >> >> The only remaining difference is the "dummy cookie can be passed >> to other functions" behavior. Which is IMHO a valid difference, >> as unlike with e.g. clk_prepare_enable(), you do pass extra data to >> request_irq(), and sometimes you do need to handle the absence of >> the interrupt using e.g. polling. >> >>> The request for a script checking this then makes sense. However, i >>> don't know how well coccinelle/sparse can track values across function >>> calls. They probably can check for: >>> >>> ret = irq_get_optional() >>> if (ret < 0) >>> return ret; >>> >>> A missing if < 0 statement somewhere later is very likely to be an >>> error. A comparison of <= 0 is also likely to be an error. A check for >>>> 0 before calling any other IRQ functions would be good. I'm >>> surprised such a check does not already existing in the IRQ API, but >>> there are probably historical reasons for that. >> >> There are still a few platforms where IRQ 0 does exist. > > Not just a few even. This happens on a reasonably recent x86 PC: > > rafael@gratch:~/work/linux-pm> head -2 /proc/interrupts > CPU0 CPU1 CPU2 CPU3 CPU4 CPU5 > 0: 10 0 0 0 0 0 > IR-IO-APIC 2-edge > timer IIRC Linus has proclaimed that IRQ0 was valid for the i8253 driver (living in arch/x86/); IRQ0 only was frowned upon when returned by platform_get_irq() and its ilk. MBR, Sergey
Hi, On 1/12/22 16:05, Sergey Shtylyov wrote: > On 1/12/22 5:41 PM, Rafael J. Wysocki wrote: > > [...] >>>>> If an optional IRQ is not present, drivers either just ignore it (e.g. >>>>> for devices that can have multiple interrupts or a single muxed IRQ), >>>>> or they have to resort to polling. For the latter, fall-back handling >>>>> is needed elsewhere in the driver. >>>>> 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. >>>>Hi, >>>> The *_optional() functions return an error code if there has been a >>>> real error which should be reported up the call stack. This excludes >>>> whatever error code indicates the requested resource does not exist, >>>> which can be -ENODEV etc. If the device does not exist, a magic cookie >>>> is returned which appears to be a valid resources but in fact is >>>> not. So the users of these functions just need to check for an error >>>> code, and fail the probe if present. >>> >>> Agreed. >>> >>> Note that in most (all?) other cases, the return type is a pointer >>> (e.g. to struct clk), and NULL is the magic cookie. >>> >>>> You seems to be suggesting in binary return value: non-zero >>>> (available) or zero (not available) >>> >>> Only in case of success. In case of a real failure, an error code >>> must be returned. >>> >>>> This discards the error code when something goes wrong. That is useful >>>> information to have, so we should not be discarding it. >>> >>> No, the error code must be retained in case of failure. >>> >>>> IRQ don't currently have a magic cookie value. One option would be to >>>> add such a magic cookie to the subsystem. Otherwise, since 0 is >>>> invalid, return 0 to indicate the IRQ does not exist. >>> >>> Exactly. And using 0 means the similar code can be used as for other >>> subsystems, where NULL would be returned. >>> >>> The only remaining difference is the "dummy cookie can be passed >>> to other functions" behavior. Which is IMHO a valid difference, >>> as unlike with e.g. clk_prepare_enable(), you do pass extra data to >>> request_irq(), and sometimes you do need to handle the absence of >>> the interrupt using e.g. polling. >>> >>>> The request for a script checking this then makes sense. However, i >>>> don't know how well coccinelle/sparse can track values across function >>>> calls. They probably can check for: >>>> >>>> ret = irq_get_optional() >>>> if (ret < 0) >>>> return ret; >>>> >>>> A missing if < 0 statement somewhere later is very likely to be an >>>> error. A comparison of <= 0 is also likely to be an error. A check for >>>>> 0 before calling any other IRQ functions would be good. I'm >>>> surprised such a check does not already existing in the IRQ API, but >>>> there are probably historical reasons for that. >>> >>> There are still a few platforms where IRQ 0 does exist. >> >> Not just a few even. This happens on a reasonably recent x86 PC: >> >> rafael@gratch:~/work/linux-pm> head -2 /proc/interrupts >> CPU0 CPU1 CPU2 CPU3 CPU4 CPU5 >> 0: 10 0 0 0 0 0 >> IR-IO-APIC 2-edge >> timer > > IIRC Linus has proclaimed that IRQ0 was valid for the i8253 driver (living in > arch/x86/); IRQ0 only was frowned upon when returned by platform_get_irq() and its > ilk. > > MBR, Sergey Right, platform_get_irq() has this: WARN(ret == 0, "0 is an invalid IRQ number\n"); So given that platform_get_irq() returning 0 is not expected, it seems reasonable for platform_get_irq_optional() to use 0 as a special "no irq available" return value, matching the NULL returned by gpiod_get_optional(). Regards, Hans
On Wed, Jan 12, 2022 at 4:14 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 1/12/22 16:05, Sergey Shtylyov wrote: > > On 1/12/22 5:41 PM, Rafael J. Wysocki wrote: > > > > [...] > >>>>> If an optional IRQ is not present, drivers either just ignore it (e.g. > >>>>> for devices that can have multiple interrupts or a single muxed IRQ), > >>>>> or they have to resort to polling. For the latter, fall-back handling > >>>>> is needed elsewhere in the driver. > >>>>> 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. > >>>>Hi, > >>>> The *_optional() functions return an error code if there has been a > >>>> real error which should be reported up the call stack. This excludes > >>>> whatever error code indicates the requested resource does not exist, > >>>> which can be -ENODEV etc. If the device does not exist, a magic cookie > >>>> is returned which appears to be a valid resources but in fact is > >>>> not. So the users of these functions just need to check for an error > >>>> code, and fail the probe if present. > >>> > >>> Agreed. > >>> > >>> Note that in most (all?) other cases, the return type is a pointer > >>> (e.g. to struct clk), and NULL is the magic cookie. > >>> > >>>> You seems to be suggesting in binary return value: non-zero > >>>> (available) or zero (not available) > >>> > >>> Only in case of success. In case of a real failure, an error code > >>> must be returned. > >>> > >>>> This discards the error code when something goes wrong. That is useful > >>>> information to have, so we should not be discarding it. > >>> > >>> No, the error code must be retained in case of failure. > >>> > >>>> IRQ don't currently have a magic cookie value. One option would be to > >>>> add such a magic cookie to the subsystem. Otherwise, since 0 is > >>>> invalid, return 0 to indicate the IRQ does not exist. > >>> > >>> Exactly. And using 0 means the similar code can be used as for other > >>> subsystems, where NULL would be returned. > >>> > >>> The only remaining difference is the "dummy cookie can be passed > >>> to other functions" behavior. Which is IMHO a valid difference, > >>> as unlike with e.g. clk_prepare_enable(), you do pass extra data to > >>> request_irq(), and sometimes you do need to handle the absence of > >>> the interrupt using e.g. polling. > >>> > >>>> The request for a script checking this then makes sense. However, i > >>>> don't know how well coccinelle/sparse can track values across function > >>>> calls. They probably can check for: > >>>> > >>>> ret = irq_get_optional() > >>>> if (ret < 0) > >>>> return ret; > >>>> > >>>> A missing if < 0 statement somewhere later is very likely to be an > >>>> error. A comparison of <= 0 is also likely to be an error. A check for > >>>>> 0 before calling any other IRQ functions would be good. I'm > >>>> surprised such a check does not already existing in the IRQ API, but > >>>> there are probably historical reasons for that. > >>> > >>> There are still a few platforms where IRQ 0 does exist. > >> > >> Not just a few even. This happens on a reasonably recent x86 PC: > >> > >> rafael@gratch:~/work/linux-pm> head -2 /proc/interrupts > >> CPU0 CPU1 CPU2 CPU3 CPU4 CPU5 > >> 0: 10 0 0 0 0 0 > >> IR-IO-APIC 2-edge > >> timer > > > > IIRC Linus has proclaimed that IRQ0 was valid for the i8253 driver (living in > > arch/x86/); IRQ0 only was frowned upon when returned by platform_get_irq() and its > > ilk. > > > > MBR, Sergey > > Right, platform_get_irq() has this: > > WARN(ret == 0, "0 is an invalid IRQ number\n"); > > So given that platform_get_irq() returning 0 is not expected, it seems > reasonable for platform_get_irq_optional() to use 0 as a special > "no irq available" return value, matching the NULL returned by > gpiod_get_optional(). Sounds reasonable to me.
On Wed, Jan 12, 2022 at 03:41:38PM +0100, Rafael J. Wysocki wrote: > On Wed, Jan 12, 2022 at 2:55 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > Hi Andrew, > > > > On Wed, Jan 12, 2022 at 2:38 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > If an optional IRQ is not present, drivers either just ignore it (e.g. > > > > for devices that can have multiple interrupts or a single muxed IRQ), > > > > or they have to resort to polling. For the latter, fall-back handling > > > > is needed elsewhere in the driver. > > > > 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. > > > > > > The *_optional() functions return an error code if there has been a > > > real error which should be reported up the call stack. This excludes > > > whatever error code indicates the requested resource does not exist, > > > which can be -ENODEV etc. If the device does not exist, a magic cookie > > > is returned which appears to be a valid resources but in fact is > > > not. So the users of these functions just need to check for an error > > > code, and fail the probe if present. > > > > Agreed. > > > > Note that in most (all?) other cases, the return type is a pointer > > (e.g. to struct clk), and NULL is the magic cookie. > > > > > You seems to be suggesting in binary return value: non-zero > > > (available) or zero (not available) > > > > Only in case of success. In case of a real failure, an error code > > must be returned. > > > > > This discards the error code when something goes wrong. That is useful > > > information to have, so we should not be discarding it. > > > > No, the error code must be retained in case of failure. > > > > > IRQ don't currently have a magic cookie value. One option would be to > > > add such a magic cookie to the subsystem. Otherwise, since 0 is > > > invalid, return 0 to indicate the IRQ does not exist. > > > > Exactly. And using 0 means the similar code can be used as for other > > subsystems, where NULL would be returned. > > > > The only remaining difference is the "dummy cookie can be passed > > to other functions" behavior. Which is IMHO a valid difference, > > as unlike with e.g. clk_prepare_enable(), you do pass extra data to > > request_irq(), and sometimes you do need to handle the absence of > > the interrupt using e.g. polling. > > > > > The request for a script checking this then makes sense. However, i > > > don't know how well coccinelle/sparse can track values across function > > > calls. They probably can check for: > > > > > > ret = irq_get_optional() > > > if (ret < 0) > > > return ret; > > > > > > A missing if < 0 statement somewhere later is very likely to be an > > > error. A comparison of <= 0 is also likely to be an error. A check for > > > > 0 before calling any other IRQ functions would be good. I'm > > > surprised such a check does not already existing in the IRQ API, but > > > there are probably historical reasons for that. > > > > There are still a few platforms where IRQ 0 does exist. > > Not just a few even. This happens on a reasonably recent x86 PC: Yes, but the timer doesn't use platform_get_irq*() and friends. > rafael@gratch:~/work/linux-pm> head -2 /proc/interrupts > CPU0 CPU1 CPU2 CPU3 CPU4 CPU5 > 0: 10 0 0 0 0 0 > IR-IO-APIC 2-edge > timer
On Wed, Jan 12, 2022 at 11:27:02AM +0100, Geert Uytterhoeven wrote: > Hi Uwe, > > On Wed, Jan 12, 2022 at 9:51 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Wed, Jan 12, 2022 at 09:33:48AM +0100, Geert Uytterhoeven wrote: > > > On Mon, Jan 10, 2022 at 10:20 PM Andrew Lunn <andrew@lunn.ch> 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/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/ > > > > > > > > > > > > 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. I agree that -ENXIO is unfortunate and -ENOENT would be more in line with other functions. I assume it's insane to want to change that. > > Is this an ack for renaming platform_get_irq_optional() to > > platform_get_irq_silent()? > > No it isn't ;-) > > If an optional IRQ is not present, drivers either just ignore it (e.g. > for devices that can have multiple interrupts or a single muxed IRQ), > or they have to resort to polling. For the latter, fall-back handling > is needed elsewhere in the driver. I think irq are not suitable for such a dummy handling. For clocks or GPIOs there are cases where just doing nothing in the absence of a certain optional clock or GPIO is fine. I checked a few users of platform_get_irq_optional() and I didn't find a single one that doesn't need to differentiate the irq and the no-irq case later. Do you know one? If you do, isn't that so exceptional that it doesn't justify the idea of a dummy irq value? So until proven otherwise I think platform_get_irq_optional() just isn't in the spirit of clk_get_optional() and gpiod_get_optional() because there are no use cases where a dummy value would be good enough. (Even if request_irq would be a noop for a dummy irq value.) The motivation why platform_get_irq_optional() was introduced was just that platform_get_irq() started to emit an error message (in commit 7723f4c5ecdb8d832f049f8483beb0d1081cedf6) and the (proportional) few drivers where the error message was bad needed a variant that doesn't emit the error message. Look at 31a8d8fa84c51d3ab00bf059158d5de6178cf890, the motivation to use platform_get_irq_optional() wasn't that it simplifies handling in the driver, but that it doesn't emit an error message. Or 8f5783ad9eb83747471f61f94dbe209fb9fb8a7d, or 2fd276c3ee4bd42eb034f8954964a5ae74187c6b, or 55cc33fab5ac9f7e2a97aa7c564e8b35355886d5. Just look at the output of git log -Splatform_get_irq_optional to find some more. That convinces me, that platform_get_irq_optional() is a bad name. The only difference to platform_get_irq is that it's silent. And returning a dummy irq value (which would make it aligned with the other _optional functions) isn't possible. > 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. Best regards Uwe
On Wed, Jan 12, 2022 at 10:31:21PM +0100, Uwe Kleine-König wrote: > On Wed, Jan 12, 2022 at 11:27:02AM +0100, Geert Uytterhoeven wrote: (Do we really need *all* the CCs here?) > That convinces me, that platform_get_irq_optional() is a bad name. The > only difference to platform_get_irq is that it's silent. And returning > a dummy irq value (which would make it aligned with the other _optional > functions) isn't possible. There is regulator_get_optional() which is I believe the earliest of these APIs, it doesn't return a dummy either (and is silent too) - this is because regulator_get() does return a dummy since it's the vastly common case that regulators must be physically present and them not being found is due to there being an error in the system description. It's unfortunate that we've ended up with these two different senses for _optional(), people frequently get tripped up by it. > > 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. Similarly for the regulator API, kind of.
On Wed, Jan 12, 2022 at 09:45:25PM +0000, Mark Brown wrote: > On Wed, Jan 12, 2022 at 10:31:21PM +0100, Uwe Kleine-König wrote: > > On Wed, Jan 12, 2022 at 11:27:02AM +0100, Geert Uytterhoeven wrote: > > (Do we really need *all* the CCs here?) It's probably counteractive to finding an agreement because there are too many opinions on that matter. But I didn't dare to strip it down, too :-) > > That convinces me, that platform_get_irq_optional() is a bad name. The > > only difference to platform_get_irq is that it's silent. And returning > > a dummy irq value (which would make it aligned with the other _optional > > functions) isn't possible. > > There is regulator_get_optional() which is I believe the earliest of > these APIs, it doesn't return a dummy either (and is silent too) - this > is because regulator_get() does return a dummy since it's the vastly > common case that regulators must be physically present and them not > being found is due to there being an error in the system description. > It's unfortunate that we've ended up with these two different senses for > _optional(), people frequently get tripped up by it. Yeah, I tripped over that one already, too. And according to my counting this results in three different senses now :-\ : a) regulator regulator_get returns a dummy, regulator_get_optional returns ERR_PTR(-ENODEV) b) clk + gpiod ..._get returns ERR_PTR(-ENODEV), ..._get_optional returns a dummy c) platform_get_irq() platform_get_irq_optional() is just a silent variant of platform_get_irq(); the return values are identical. 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.) 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. Best regards Uwe
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. > 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.
On 1/13/22 12:45 AM, Mark Brown wrote: [...] > (Do we really need *all* the CCs here?) Yeah, 25 files were changed and that resulted in 75 persons/lists addressed. I didn't expect such a wide audience myself... :-) >> That convinces me, that platform_get_irq_optional() is a bad name. The >> only difference to platform_get_irq is that it's silent. And returning >> a dummy irq value (which would make it aligned with the other _optional >> functions) isn't possible. > There is regulator_get_optional() which is I believe the earliest of > these APIs, it doesn't return a dummy either (and is silent too) - this Hm, I'm seeing it's rather noisy... :-) > is because regulator_get() does return a dummy since it's the vastly > common case that regulators must be physically present and them not > being found is due to there being an error in the system description. > It's unfortunate that we've ended up with these two different senses for > _optional(), people frequently get tripped up by it. > >>> 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/... > Similarly for the regulator API, kind of. MBR, Sergey
Hello Sergey, On Thu, Jan 13, 2022 at 11:35:34PM +0300, Sergey Shtylyov wrote: > On 1/13/22 12:45 AM, Mark Brown 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(), 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: 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) { ... 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". Note the win is considerably less compared to gpiod_get_optional vs gpiod_get however. And then it still lacks the semantic of a dummy irq which IMHO forfeits the right to call it ..._optional(). Now I'm unwilling to continue the discussion unless there pops up a suggestion that results in a considerable part (say > 10%) of the drivers using platform_get_irq_optional not having to check if the return value corresponds to "not found". Best regards Uwe
Hi Uwe, On Fri, Jan 14, 2022 at 10:26 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Thu, Jan 13, 2022 at 11:35:34PM +0300, Sergey Shtylyov wrote: > > On 1/13/22 12:45 AM, Mark Brown 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(), > 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: > 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) { > ... 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". Please don't make that change. If platform_get_irq() would return 0 on "not found", all existing users have to be changed to: irq = platform_get_irq(...); if (irq < 0) { return irq; } else if (!irq) { return -ENOENT; } else { ... setup irq operation ... } If the IRQ is not optional, there should be an error code when it is not present. This keeps error handling simple. The _optional() difference lies in the zero/NULL vs. error code in case of not present. > Note the win is considerably less compared to gpiod_get_optional vs > gpiod_get however. And then it still lacks the semantic of a dummy irq > which IMHO forfeits the right to call it ..._optional(). > > Now I'm unwilling to continue the discussion unless there pops up a > suggestion that results in a considerable part (say > 10%) of the > drivers using platform_get_irq_optional not having to check if the > return value corresponds to "not found". Usually drivers do have to check if the interrupt was present or not, because they may have to change the operation of the driver, depending on interrupt-based or timer/polling-based processing. Clocks, regulators, and resets are different, as their absence is really a no-op. The absence of an interrupt is not a no-op (except for the separate interrupts vs. a single muxed interrupt case). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
On 1/13/22 11:35 PM, Sergey Shtylyov wrote: [...] >> (Do we really need *all* the CCs here?) > > Yeah, 25 files were changed and that resulted in 75 persons/lists addressed. > I didn't expect such a wide audience myself... :-) And, of course, I specified --nogit-fallback to scripts/get_maintainers.pl, so there's no random people... [...] MBR, Sergey
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... > 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? > 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). > ... 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? 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 ;-)). > 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? > Now I'm unwilling to continue the discussion unless there pops up a > suggestion that results in a considerable part (say > 10%) of the > drivers using platform_get_irq_optional not having to check if the > return value corresponds to "not found". Note that many actual drivers don't follow the pattern prescribed in the comment to platform_get_irq_optional() and their check for an optional IRQ look like irq < 0 (and, after my patches, irq <= 0). Maybe we shouldn't even bother returning the error codes and just return 0 for all kinds of misfortunes instead? :-) > Best regards > Uwe MBR, Sergey
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
Hello, I'm trying to objectively summarize the discussions in this thread in the hope this helps finding a way that most people can live with. First a description of the status quo: There are several function pairs *get() and *get_optional() that however are different in various aspects. Their relevant properties are listes in the following table. Ideally each line had only identical entries. | clk_get | gpiod_get | platform_get_irq | regulator_get | return value | | | | | on not-found | ERR_PTR(-ENOENT) | ERR_PTR(-ENOENT) | -ENXIO | dummy[1] | (plain get) | | | | | | | | | | return value | | | | | on not-found | dummy[1] | dummy[1] | -ENXIO | ERR_PTR(-ENOENT) | (get_optional) | | | | | | | | | | emits an error message | | | | | on error (including | no | no | yes[2] | no | not-found) | | | | | | | | | | get_optional emits an error | | | | | message on error (including | no | no | no | no | not-found) | | | | | | | | | | summary: | returning a dummy | returning a dummy | doesn't emit an | returning error code | *_get_optional() differs from | on not-found | on not-found | error message | on not-found | *_get by: | | | | | [1] the dummy value is a valid resource descriptor, the API functions are a noop for this dummy value. This dummy value is NULL for all three subsystems. [2] no error is printed for -EPROBE_DEFER. The inversion between clk+gpio vs. regulator is unforunate, swaping one or the other would be good for consistency, but this isn't the topic of this thread. Only so much: It's not agreed upon which variant is the better one and the difference is of historical origin. There are now different suggestions to improve the situation regarding platform_get_irq() compared to the other functions: a) by Sergey platform_get_irq_optional() is changed to return 0 on not-found. b) by Uwe rename platform_get_irq_optional() to platform_get_irq_silent() The argument pro a) is: platform_get_irq_optional() is aligned to clk_get() and gpiod_get() by returning 0 on not-found. The argument contra a) The return value 0 for platform_get_irq() is only syntactically nearer to the dummy value of clk_get() and gpiod_get(). A dummy value isn't available and probably not sensible to introduce for irq because most drivers have to check for the not-found situation anyhow to setup polling. The argument pro b) is: The relevant difference between platform_get_irq() and its optional variant is that the latter is silent. This is a different concept for the meaning of optional compared to the other *_get_optional(). The argument contra b) is: The chosen name is bad, because driver authors might wonder what a silent irq is. ---- end of summary 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). Best regards Uwe
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
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
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
On Sat, Jan 15, 2022 at 9:22 PM Sergey Shtylyov <s.shtylyov@omp.ru> 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 -- geert@linux-m68k.org 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
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
Hello Geert, On Mon, Jan 17, 2022 at 09:41:42AM +0100, Geert Uytterhoeven wrote: > On Sat, Jan 15, 2022 at 9:22 PM Sergey Shtylyov <s.shtylyov@omp.ru> 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. 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. 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. 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. Best regards Uwe
Hi Uwe, On Mon, Jan 17, 2022 at 10:24 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 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 <s.shtylyov@omp.ru> 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. 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 ;-) > 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. > 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. 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. Commit 8973ea47901c81a1 ("driver core: platform: Introduce platform_get_irq_optional()") fixed (b), but didn't address (a) and (c). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
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 > <u.kleine-koenig@pengutronix.de> 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 <s.shtylyov@omp.ru> 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
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/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/ > > 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 <matthias.schiffer@ew.tq-group.com> > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> [...] 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
Hi Uwe, On Mon, Jan 17, 2022 at 12:49 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 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 > > <u.kleine-koenig@pengutronix.de> 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 <s.shtylyov@omp.ru> 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 -- geert@linux-m68k.org 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
On Mon, Jan 17, 2022 at 02:08:19PM +0100, Geert Uytterhoeven wrote: > Hi Uwe, > > On Mon, Jan 17, 2022 at 12:49 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> 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 > > > <u.kleine-koenig@pengutronix.de> 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 <s.shtylyov@omp.ru> 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. We agree that one of the two functions is enough, just differ in which of the two we want to have. :-) If you think platform_get_irq_silently() is a good intermediate step for your goal, then we agree to rename platform_get_irq_optional(). So I suggest you ack my patch. > Still, the rename would touch all users at once anyway. It would be more easy to keep the conversion regression-free however. A plain rename is simple to verify. And then converting to the new platform_get_irq_optional() can be done individually and without the need to do everything in a single step. > > 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. If you want to make platform_get_irq and its optional variant more similar to the others, dropping the error message is the way to go. > > > > 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) { This is not a simplification, just looking at the line count and the added gotos. That's because it also improves error handling and so the effect isn't easily spotted. > I like it when the "if (ret < ) ..." error handling is the first check to do. That's a relevant difference between us. > 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; > } I would do the latter, then it's in the normal order for error handling handle some specific errors; forward unhandled errors up the stack; handle success; but it seems you prefer to not call "not found" an error. Actually I think it's an advantage that the driver has to mention -ENXIO, feels like proper error handling to me. I guess we won't agree about that though. What about the following idea (in pythonic pseudo code for simplicity): # the rspi device either has two irqs, one for rx and one for # tx, or a single one for both together. def muxed_hander(irq): status = readl(STATUS) if status & IF_RX: rx_handler() if status & IF_TX: tx_handler() def probe_muxed_irq(): irq = platform_get_irq_by_name("mux") if irq < 0: return irq; request_irq(irq, muxed_handler) def probe_separate_irqs(): txirq = platform_get_irq_by_name("tx") if txirq < 0: return txirq rxirq = platform_get_irq_by_name("rx") if rxirq < 0: return rxirq request_irq(txirq, tx_handler) request_irq(rxirq, rx_handler) def probe(): ret = probe_separate_irqs() if ret == -ENXIO: ret = probe_muxed_irq() if ret < 0: return ret looks clean (to me that is) and allows to skip the demuxing in tx_handler and rx_handler (which might or might not yield improved runtime behaviour). Maybe a bit more verbose, but simpler to grasp for a human, isn't it? > 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. Yeah, the issue where we don't agree is if "not-found" is special enough to deserve the natural magic value. For me -ENXIO is magic enough to handle the absence of an irq line. I consider it even the better magic value. > > (c) still has to be proven, see above. Best regards Uwe
Hi Uwe, On Mon, Jan 17, 2022 at 6:06 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Mon, Jan 17, 2022 at 02:08:19PM +0100, Geert Uytterhoeven wrote: > > On Mon, Jan 17, 2022 at 12:49 PM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > > 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. > > > 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. [...] > This is not a simplification, just looking at the line count and the > added gotos. That's because it also improves error handling and so the > effect isn't easily spotted. Yes, it's larger because it adds currently missing error handling. > What about the following idea (in pythonic pseudo code for simplicity): No idea what you gain by throwing in a language that is irrelevant to kernel programming (why no Rust? ;-) > > > > 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. > > Yeah, the issue where we don't agree is if "not-found" is special enough > to deserve the natural magic value. For me -ENXIO is magic enough to > handle the absence of an irq line. I consider it even the better magic > value. It differs from other subsystems (clk, gpio, reset), which do return zero on not found. What's the point in having *_optional() APIs if they just return the same values as the non-optional ones? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
Hello Geert, On Tue, Jan 18, 2022 at 09:25:01AM +0100, Geert Uytterhoeven wrote: > On Mon, Jan 17, 2022 at 6:06 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Mon, Jan 17, 2022 at 02:08:19PM +0100, Geert Uytterhoeven wrote: > > > On Mon, Jan 17, 2022 at 12:49 PM Uwe Kleine-König > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > 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. > > > > Yeah, the issue where we don't agree is if "not-found" is special enough > > to deserve the natural magic value. For me -ENXIO is magic enough to > > handle the absence of an irq line. I consider it even the better magic > > value. > > It differs from other subsystems (clk, gpio, reset), which do return > zero on not found. IMHO it doesn't matter at all that the return value is zero, relevant is the semantic of the returned value. For clk, gpio, reset and regulator NULL is a usable dummy, for irqs it's not. So what you do with the value returned by platform_get_irq_whatever() is: you compare it with the (magic?) not-found value, and if it matches, you enter a suitable if-block. For the (clk|gpiod|regulator)_get_optional() you don't have to check against the magic not-found value (so no implementation detail magic leaks into the caller code) and just pass it to the next API function. (And my expectation would be that if you chose to represent not-found by (void *)66 instead of NULL, you won't have to adapt any user, just the framework internal checks. This is a good thing!) > What's the point in having *_optional() APIs if they just return the > same values as the non-optional ones? The upside is that functions with a similar name have similar semantics. But I agree, having platform_get_irq_optional() with the same return value for not-found is bad. Changing the return semantic is only one way to cope with that, renaming such the actual semantic difference is obvious from the function name is another (IMHO better one). Best regards Uwe
On Sun, Jan 16, 2022 at 03:19:06PM +0100, Greg Kroah-Hartman wrote: > 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. Fine for me. I assume this is a Nack for Sergey's patch? Best regards Uwe
On Tue, Jan 18, 2022 at 10:18:19AM +0100, Uwe Kleine-König wrote: > On Sun, Jan 16, 2022 at 03:19:06PM +0100, Greg Kroah-Hartman wrote: > > 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. > > Fine for me. I assume this is a Nack for Sergey's patch? This set of patches is going nowhere as-is, sorry. The thread is too confusing and people are not agreeing at all. greg k-h
Hi Uwe, On Tue, Jan 18, 2022 at 10:09 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Tue, Jan 18, 2022 at 09:25:01AM +0100, Geert Uytterhoeven wrote: > > On Mon, Jan 17, 2022 at 6:06 PM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > On Mon, Jan 17, 2022 at 02:08:19PM +0100, Geert Uytterhoeven wrote: > > > > On Mon, Jan 17, 2022 at 12:49 PM Uwe Kleine-König > > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > 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. > > > > > > Yeah, the issue where we don't agree is if "not-found" is special enough > > > to deserve the natural magic value. For me -ENXIO is magic enough to > > > handle the absence of an irq line. I consider it even the better magic > > > value. > > > > It differs from other subsystems (clk, gpio, reset), which do return > > zero on not found. > > IMHO it doesn't matter at all that the return value is zero, relevant is > the semantic of the returned value. For clk, gpio, reset and regulator > NULL is a usable dummy, for irqs it's not. So what you do with the value > returned by platform_get_irq_whatever() is: you compare it with the > (magic?) not-found value, and if it matches, you enter a suitable > if-block. > > For the (clk|gpiod|regulator)_get_optional() you don't have to check > against the magic not-found value (so no implementation detail magic > leaks into the caller code) and just pass it to the next API function. > (And my expectation would be that if you chose to represent not-found by > (void *)66 instead of NULL, you won't have to adapt any user, just the > framework internal checks. This is a good thing!) Ah, there is the wrong assumption: drivers sometimes do need to know if the resource was found, and thus do need to know about (void *)66, -ENODEV, or -ENXIO. I already gave examples for IRQ and clk before. I can imagine these exist for gpiod and regulator, too, as soon as you go beyond the trivial "enable" and "disable" use-cases. And 0/NULL vs. > 0 is the natural check here: missing, but not an error. Even for IRQ this was envisioned before, when it was decided that vIRQ zero does not exist. (Inconsistent) Error codes are not, as missing optional resources are not error conditions. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
Hello Geert, On Tue, Jan 18, 2022 at 10:37:25AM +0100, Geert Uytterhoeven wrote: > On Tue, Jan 18, 2022 at 10:09 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > For the (clk|gpiod|regulator)_get_optional() you don't have to check > > against the magic not-found value (so no implementation detail magic > > leaks into the caller code) and just pass it to the next API function. > > (And my expectation would be that if you chose to represent not-found by > > (void *)66 instead of NULL, you won't have to adapt any user, just the > > framework internal checks. This is a good thing!) > > Ah, there is the wrong assumption: drivers sometimes do need to know > if the resource was found, and thus do need to know about (void *)66, > -ENODEV, or -ENXIO. I already gave examples for IRQ and clk before. > I can imagine these exist for gpiod and regulator, too, as soon as > you go beyond the trivial "enable" and "disable" use-cases. My premise is that every user who has to check for "not found" explicitly should not use (clk|gpiod)_get_optional() but (clk|gpiod)_get() and do proper (and explicit) error handling for -ENODEV. (clk|gpiod)_get_optional() is only for these trivial use-cases. > And 0/NULL vs. > 0 is the natural check here: missing, but not > an error. For me it it 100% irrelevant if "not found" is an error for the query function or not. I just have to be able to check for "not found" and react accordingly. And adding a function def platform_get_irq_opional(): ret = platform_get_irq() if ret == -ENXIO: return 0 return ret it's not a useful addition to the API if I cannot use 0 as a dummy because it doesn't simplify the caller enough to justify the additional function. The only thing I need to be able is to distinguish the cases "there is an irq", "there is no irq" and anything else is "there is a problem I cannot handle and so forward it to my caller". The semantic of platform_get_irq() is able to satisfy this requirement[1], so why introduce platform_get_irq_opional() for the small advantage that I can check for not-found using if (!irq) instead of if (irq != -ENXIO) ? The semantic of platform_get_irq() is easier ("Either a usable non-negative irq number or a negative error number") compared to platform_get_irq_optional() ("Either a usable positive irq number or a negative error number or 0 meaning not found"). Usage of platform_get_irq() isn't harder or more expensive (neither for a human reader nor for a maching running the resulting compiled code). For a human reader if (irq != -ENXIO) is even easier to understand because for if (!irq) they have to check where the value comes from, see it's platform_get_irq_optional() and understand that 0 means not-found. This function just adds overhead because as a irq framework user I have to understand another function. For me the added benefit is too small to justify the additional function. And you break out-of-tree drivers. These are all no major counter arguments, but as the advantage isn't major either, they still matter. Best regards Uwe [1] the only annoying thing is the error message.
Hi Uwe, On Tue, Jan 18, 2022 at 1:08 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Tue, Jan 18, 2022 at 10:37:25AM +0100, Geert Uytterhoeven wrote: > > On Tue, Jan 18, 2022 at 10:09 AM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > For the (clk|gpiod|regulator)_get_optional() you don't have to check > > > against the magic not-found value (so no implementation detail magic > > > leaks into the caller code) and just pass it to the next API function. > > > (And my expectation would be that if you chose to represent not-found by > > > (void *)66 instead of NULL, you won't have to adapt any user, just the > > > framework internal checks. This is a good thing!) > > > > Ah, there is the wrong assumption: drivers sometimes do need to know > > if the resource was found, and thus do need to know about (void *)66, > > -ENODEV, or -ENXIO. I already gave examples for IRQ and clk before. > > I can imagine these exist for gpiod and regulator, too, as soon as > > you go beyond the trivial "enable" and "disable" use-cases. > > My premise is that every user who has to check for "not found" > explicitly should not use (clk|gpiod)_get_optional() but > (clk|gpiod)_get() and do proper (and explicit) error handling for > -ENODEV. (clk|gpiod)_get_optional() is only for these trivial use-cases. > > > And 0/NULL vs. > 0 is the natural check here: missing, but not > > an error. > > For me it it 100% irrelevant if "not found" is an error for the query > function or not. I just have to be able to check for "not found" and > react accordingly. > > And adding a function > > def platform_get_irq_opional(): > ret = platform_get_irq() > if ret == -ENXIO: > return 0 > return ret > > it's not a useful addition to the API if I cannot use 0 as a dummy > because it doesn't simplify the caller enough to justify the additional > function. > > The only thing I need to be able is to distinguish the cases "there is > an irq", "there is no irq" and anything else is "there is a problem I > cannot handle and so forward it to my caller". The semantic of > platform_get_irq() is able to satisfy this requirement[1], so why introduce > platform_get_irq_opional() for the small advantage that I can check for > not-found using > > if (!irq) > > instead of > > if (irq != -ENXIO) > > ? The semantic of platform_get_irq() is easier ("Either a usable > non-negative irq number or a negative error number") compared to > platform_get_irq_optional() ("Either a usable positive irq number or a > negative error number or 0 meaning not found"). Usage of > platform_get_irq() isn't harder or more expensive (neither for a human > reader nor for a maching running the resulting compiled code). > For a human reader > > if (irq != -ENXIO) > > is even easier to understand because for > > if (!irq) > > they have to check where the value comes from, see it's > platform_get_irq_optional() and understand that 0 means not-found. "vIRQ zero does not exist." > This function just adds overhead because as a irq framework user I have > to understand another function. For me the added benefit is too small to > justify the additional function. And you break out-of-tree drivers. > These are all no major counter arguments, but as the advantage isn't > major either, they still matter. > > Best regards > Uwe > > [1] the only annoying thing is the error message. So there's still a need for two functions. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
On Tue, Jan 18, 2022 at 01:49:15PM +0100, Geert Uytterhoeven wrote: > nst the magic not-found value (so no implementation detail magic > > > > leaks into the caller code) and just pass it to the next API function= > . > > > > (And my expectation would be that if you chose to represent not-found= > by > > > > (void *)66 instead of NULL, you won't have to adapt any user, just th= > e > > > > framework internal checks. This is a good thing!) > > > > > > Ah, there is the wrong assumption: drivers sometimes do need to know > > > if the resource was found, and thus do need to know about (void *)66, > > > -ENODEV, or -ENXIO. I already gave examples for IRQ and clk before. > > > I can imagine these exist for gpiod and regulator, too, as soon as > > > you go beyond the trivial "enable" and "disable" use-cases. > > > > My premise is that every user who has to check for "not found" > > explicitly should not use (clk|gpiod)_get_optional() but > > (clk|gpiod)_get() and do proper (and explicit) error handling for > > -ENODEV. (clk|gpiod)_get_optional() is only for these trivial use-cases. > > > > > And 0/NULL vs. > 0 is the natural check here: missing, but not > > > an error. > > > > For me it it 100% irrelevant if "not found" is an error for the query > > function or not. I just have to be able to check for "not found" and > > react accordingly. > > > > And adding a function > > > > def platform_get_irq_opional(): > > ret =3D platform_get_irq() > > if ret =3D=3D -ENXIO: > > return 0 > > return ret > > > > it's not a useful addition to the API if I cannot use 0 as a dummy > > because it doesn't simplify the caller enough to justify the additional > > function. > > > > The only thing I need to be able is to distinguish the cases "there is > > an irq", "there is no irq" and anything else is "there is a problem I > > cannot handle and so forward it to my caller". The semantic of > > platform_get_irq() is able to satisfy this requirement[1], so why introdu= > ce > > platform_get_irq_opional() for the small advantage that I can check for > > not-found using > > > > if (!irq) > > > > instead of > > > > if (irq !=3D -ENXIO) > > > > ? The semantic of platform_get_irq() is easier ("Either a usable > > non-negative irq number or a negative error number") compared to > > platform_get_irq_optional() ("Either a usable positive irq number or a > > negative error number or 0 meaning not found"). Usage of > > platform_get_irq() isn't harder or more expensive (neither for a human > > reader nor for a maching running the resulting compiled code). > > For a human reader > > > > if (irq !=3D -ENXIO) > > > > is even easier to understand because for > > > > if (!irq) > > > > they have to check where the value comes from, see it's > > platform_get_irq_optional() and understand that 0 means not-found. > > "vIRQ zero does not exist." With that statement in mind I would expect that a function that gives me an (v)irq number never returns 0. > > This function just adds overhead because as a irq framework user I have > > to understand another function. For me the added benefit is too small to > > justify the additional function. And you break out-of-tree drivers. > > These are all no major counter arguments, but as the advantage isn't > > major either, they still matter. > > > > Best regards > > Uwe > > > > [1] the only annoying thing is the error message. > > So there's still a need for two functions. Or a single function not emitting an error message together with the callers being responsible for calling dev_err(). So the options in my preference order (first is best) are: - Remove the printk from platform_get_irq() and remove platform_get_irq_optional(); - Rename platform_get_irq_optional() to platform_get_irq_silently() - Keep platform_get_irq_optional() as is - Collect underpants - ? - Change semantic of platform_get_irq_optional() Best regards Uwe
Hello! On 1/17/22 11:47 AM, 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'm doing it first already: https://lore.kernel.org/all/5e001ec1-d3f1-bcb8-7f30-a6301fd9930c@omp.ru/ This series is atop of the above patch... > I didn't recheck, but is this what the > driver changes in your patch is about? Partly, yes. We can afford to play with the meaning of 0 after the above patch. > 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 Mainly, yes. That's why the code examples were given in the description. > 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 ? */ Not >= 0, no... > 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. I think comparing with 0 is simpler (and shorter) than with -ENXIO, if you consider the RISC CPUs, like e.g. MIPS... > 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. Mhm, I did include all the drivers where the IRQ checks have to be modified, not sure what else you want me to touch... > Best regards > Uwe MBR, Sergey
On Tue, Jan 18, 2022 at 11:21:45PM +0300, Sergey Shtylyov wrote: > Hello! > > On 1/17/22 11:47 AM, 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'm doing it first already: > > https://lore.kernel.org/all/5e001ec1-d3f1-bcb8-7f30-a6301fd9930c@omp.ru/ > > This series is atop of the above patch... Ah, I missed that (probably because I didn't get the cover letter). > > I didn't recheck, but is this what the > > driver changes in your patch is about? > > Partly, yes. We can afford to play with the meaning of 0 after the above patch. But the changes that are in patch 1 are all needed? > > 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 > > Mainly, yes. That's why the code examples were given in the description. > > > 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 ? */ > > Not >= 0, no... > > > 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. > > I think comparing with 0 is simpler (and shorter) than with -ENXIO, if you > consider the RISC CPUs, like e.g. MIPS... Hmm, I don't know MIPS good enough to judge. So I created a small C file: $ cat test.c #include <errno.h> int platform_get_irq_optional(void); void a(void); int func_0() { int irq = platform_get_irq_optional(); if (irq == 0) a(); } int func_enxio() { int irq = platform_get_irq_optional(); if (irq == -ENXIO) a(); } With some cross compilers as provided by Debian doing $CC -c -O3 test.c nm --size-sort test.o I get: compiler | size of func_0 | size of func_enxio ================================+==================|==================== aarch64-linux-gnu-gcc | 0000000000000024 | 0000000000000028 arm-linux-gnueabi-gcc | 00000018 | 00000018 arm-linux-gnueabihf-gcc | 00000010 | 00000012 i686-linux-gnu-gcc | 0000002a | 0000002a mips64el-linux-gnuabi64-gcc | 0000000000000054 | 000000000000005c powerpc-linux-gnu-gcc | 00000058 | 00000058 s390x-linux-gnu-gcc | 000000000000002e | 0000000000000030 x86_64-linux-gnu-gcc | 0000000000000022 | 0000000000000022 So you save some bytes indeed. > > 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. > > Mhm, I did include all the drivers where the IRQ checks have to be modified, > not sure what else you want me to touch... I somehow expected that the changes that are now necessary (or possible) to callers makes them prettier somehow. Looking at your patch again: - drivers/counter/interrupt-cnt.c This one is strange in my eyes because it tests the return value of gpiod_get_optional against NULL :-( - drivers/edac/xgene_edac.c This one just wants a silent irq lookup and then throws away the error code returned by platform_get_irq_optional() to return -EINVAL. Not so nice, is it? - drivers/gpio/gpio-altera.c This one just wants a silent irq lookup. And maybe it should only goto skip_irq if the irq was not found, but on an other error code abort the probe?! - drivers/gpio/gpio-mvebu.c Similar to gpio-altera.c: Wants a silent irq and improved error handling. - drivers/i2c/busses/i2c-brcmstb.c A bit ugly that we now have dev->irq == 0 if the irq isn't available, but if requesting the irq failed irq = -1 is used? - drivers/mmc/host/sh_mmcif.c Broken error handling. This one wants to abort on irq[1] < 0 (with your changed semantic). I stopped here. It seems quite common that drivers assume a value < 0 returned by platform_get_irq means not-found and don't care for -EPROBE_DEFER (what else can happen?) Changing a relevant function in that mess seems unfortunate here :-\ Best regards Uwe
On 1/18/22 12:18 PM, Uwe Kleine-König wrote: > On Sun, Jan 16, 2022 at 03:19:06PM +0100, Greg Kroah-Hartman wrote: >> 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. > > Fine for me. I assume this is a Nack for Sergey's patch? Which patch do you mean? I'm starting to get really muddled... :-( > Best regards > Uwe MBR, Sergey
Hello Sergey, On Wed, Jan 19, 2022 at 01:56:12PM +0300, Sergey Shtylyov wrote: > On 1/18/22 12:18 PM, Uwe Kleine-König wrote: > > On Sun, Jan 16, 2022 at 03:19:06PM +0100, Greg Kroah-Hartman wrote: > >> 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. > > > > Fine for me. I assume this is a Nack for Sergey's patch? > > Which patch do you mean? I'm starting to get really muddled... :-( I'm talking about "[PATCH 1/2] platform: make platform_get_irq_optional() optional" because "trying to forward-port an old driver will miss the abi" applies to it. Best regards Uwe
On 1/19/22 2:33 PM, 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. >>> >>> Fine for me. I assume this is a Nack for Sergey's patch? >> >> Which patch do you mean? I'm starting to get really muddled... :-( > > I'm talking about "[PATCH 1/2] platform: make > platform_get_irq_optional() optional" I thought GregKH was talking about your renaming patch... :-/ > because "trying to forward-port an > old driver will miss the abi" applies to it. Mhm... why not tell me right from the start? Jr even tell that to Andy instead of merging his patch, so I wouldn't get sucked into this work? I wouldn't bother with v2 and it would have saved a lot of time spent on email... :-( Do we also remember that "the stable API is a nonsense" thing? :-) > Best regards > Uwe MBR, Sergey
On Mon, Jan 17, 2022 at 02:57:32PM +0300, Sergey Shtylyov wrote: > 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/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/ > > > > 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 <matthias.schiffer@ew.tq-group.com> > > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> > [...] > > Please don't merge this as yet, I'm going thru this patch once again > and have already found some sloppy code. :-/ Who would you expect to merge this? I would have expected Greg, but he seems to have given up this thread. > > 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. I didn't think about what you mean with sloppy, but the code is equivalent to if (bt_bmc->irq <= 0) return bt_bmc->irq; > > > > 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? _optinal isn't correct, _optional maybe is. :-) Anyhow, look at e26124cd5f7099949109608845bba9e9bf96599c, the driver was fixed not to print two error messages and the wrong option was picked. > > - if (irq < 0) { > > + if (irq <= 0) { > > dev_err(&pdev->dev, "No IRQ resource\n"); > > rc = -EINVAL; > > goto out_err; What's wrong here is that the return code is hardcoded ... > [...] > > 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; Ack, could be simplified by switching to platform_get_irq(). Best regards Uwe
Hello! On 1/19/22 1:26 AM, Uwe Kleine-König wrote: [...] >>>>>>> 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'm doing it first already: >> >> https://lore.kernel.org/all/5e001ec1-d3f1-bcb8-7f30-a6301fd9930c@omp.ru/ >> >> This series is atop of the above patch... > > Ah, I missed that (probably because I didn't get the cover letter). > >>> I didn't recheck, but is this what the >>> driver changes in your patch is about? >> >> Partly, yes. We can afford to play with the meaning of 0 after the above patch. > > But the changes that are in patch 1 are all needed? Yes, they follow from the platform_get_irq_optional() changing the sense of its result... [...] >>> 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 ? */ >> >> Not >= 0, no... >> >>> 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. >> >> I think comparing with 0 is simpler (and shorter) than with -ENXIO, if you >> consider the RISC CPUs, like e.g. MIPS... > > Hmm, I don't know MIPS good enough to judge. So I created a small C MIPS has read-only register 0 (containing 0 :-)) which should simplify things. But I'd have to check the actual object code... yeah, MIPS has a branching instruction that compares 2 registers and branches on the result'; with -ENXIO you'd have to load an immediate value into a register first... > file: > > $ cat test.c > #include <errno.h> > > int platform_get_irq_optional(void); > void a(void); > > int func_0() > { > int irq = platform_get_irq_optional(); > > if (irq == 0) > a(); > } > > int func_enxio() > { > int irq = platform_get_irq_optional(); > > if (irq == -ENXIO) > a(); > } > > With some cross compilers as provided by Debian doing > > $CC -c -O3 test.c Mhm, do we really use -O3 to build the kernel? > nm --size-sort test.o > > I get: > > compiler | size of func_0 | size of func_enxio > ================================+==================|==================== > aarch64-linux-gnu-gcc | 0000000000000024 | 0000000000000028 > arm-linux-gnueabi-gcc | 00000018 | 00000018 > arm-linux-gnueabihf-gcc | 00000010 | 00000012 Hm, 2 bytes only -- perhaps Thumb mode? > i686-linux-gnu-gcc | 0000002a | 0000002a Expected. > mips64el-linux-gnuabi64-gcc | 0000000000000054 | 000000000000005c That's even more than expected -- 64-bit mode used? > powerpc-linux-gnu-gcc | 00000058 | 00000058 Well, they say > s390x-linux-gnu-gcc | 000000000000002e | 0000000000000030 > x86_64-linux-gnu-gcc | 0000000000000022 | 0000000000000022 Again, as expected... > So you save some bytes indeed. I see you have a lot of spare time (unlike me!). :-) >>> 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. >> >> Mhm, I did include all the drivers where the IRQ checks have to be modified, >> not sure what else you want me to touch... > > I somehow expected that the changes that are now necessary (or possible) > to callers makes them prettier somehow. Looking at your patch again: I think they do... > > - drivers/counter/interrupt-cnt.c > This one is strange in my eyes because it tests the return value of > gpiod_get_optional against NULL :-( Mhm, how is this connected with my patch? :-/ > - drivers/edac/xgene_edac.c > This one just wants a silent irq lookup and then throws away the > error code returned by platform_get_irq_optional() to return -EINVAL. > Not so nice, is it? I have dropped this file from my (to be posted yet) v2... sorry that it took so long... > - drivers/gpio/gpio-altera.c > This one just wants a silent irq lookup. And maybe it should only > goto skip_irq if the irq was not found, but on an other error code > abort the probe?! That's debatable... but anyway it's a matter of a separate (follow up) patch... > > - drivers/gpio/gpio-mvebu.c > Similar to gpio-altera.c: Wants a silent irq and improved error > handling. Same as above... > - drivers/i2c/busses/i2c-brcmstb.c > A bit ugly that we now have dev->irq == 0 if the irq isn't available, > but if requesting the irq failed irq = -1 is used? This doesn't matter much really but can change to 0, if you want... :-) > > - drivers/mmc/host/sh_mmcif.c > Broken error handling. This one wants to abort on irq[1] < 0 (with > your changed semantic). Again, matter of a separate patch (I don't have the guily hardware anymore but I guess Geert could help with that). > I stopped here. Note that most of your complaints are about the existing driver logic -- which my patch just couldn't deal with... I currently don't have the bandwidth for addressing all your complaints; some (more obvious) I'm goiing to address as the time permits, the draft patches have been done already... > It seems quite common that drivers assume a value < 0 returned by > platform_get_irq means not-found Of course, before this patch -ENXIO meant IRQ-not-found, many drivers don't bother to filter it out separately (for simplicity?). > and don't care for -EPROBE_DEFER (what else can happen?). Hm, I haven't really seen a lot the probe dererral mishandling in the code touched by at least my patch #1... > Changing a relevant function in that mess seems unfortunate here :-\ You seem to have some spare time and I'm getting distracted contrariwise... want to help? :-) > Best regards > Uwe MBR, Sergey
On 1/19/22 6:02 PM, Uwe Kleine-König wrote: [...] >>> This patch is based on the former Andy Shevchenko's patch: >>> >>> https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/ >>> >>> 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 <matthias.schiffer@ew.tq-group.com> >>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> >> [...] >> >> Please don't merge this as yet, I'm going thru this patch once again >> and have already found some sloppy code. :-/ > > Who would you expect to merge this? I would have expected Greg, but he Me too, it's his area, the message was addressed to Greg KH... > seems to have given up this thread. You instill too much uncertainty in him. :-) >>> 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. > > I didn't think about what you mean with sloppy, but the code is > equivalent to > > if (bt_bmc->irq <= 0) > return bt_bmc->irq; Exactly. [...] >>> 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? > > _optinal isn't correct, _optional maybe is. :-) No. :-) > Anyhow, look at e26124cd5f7099949109608845bba9e9bf96599c, the driver was > fixed not to print two error messages and the wrong option was picked. I think this patch is wrong... >>> - if (irq < 0) { >>> + if (irq <= 0) { >>> dev_err(&pdev->dev, "No IRQ resource\n"); This is what needed to be thrown overboard... :-) >>> rc = -EINVAL; >>> goto out_err; > > What's wrong here is that the return code is hardcoded ... This is wrong as well -- kills the deferred probing. I have 2 separate patches for this driver now... just need some time to get 'em ready... [...] >>> 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; > > Ack, could be simplified by switching to platform_get_irq(). Have a draft patch... > Best regards > Uwe MBR, Sergey
On 1/18/22 5:29 PM, Uwe Kleine-König wrote: >> nst the magic not-found value (so no implementation detail magic >>>>> leaks into the caller code) and just pass it to the next API function= >> . >>>>> (And my expectation would be that if you chose to represent not-found= >> by >>>>> (void *)66 instead of NULL, you won't have to adapt any user, just th= >> e >>>>> framework internal checks. This is a good thing!) >>>> >>>> Ah, there is the wrong assumption: drivers sometimes do need to know >>>> if the resource was found, and thus do need to know about (void *)66, >>>> -ENODEV, or -ENXIO. I already gave examples for IRQ and clk before. >>>> I can imagine these exist for gpiod and regulator, too, as soon as >>>> you go beyond the trivial "enable" and "disable" use-cases. >>> >>> My premise is that every user who has to check for "not found" >>> explicitly should not use (clk|gpiod)_get_optional() but >>> (clk|gpiod)_get() and do proper (and explicit) error handling for >>> -ENODEV. (clk|gpiod)_get_optional() is only for these trivial use-cases. >>> >>>> And 0/NULL vs. > 0 is the natural check here: missing, but not >>>> an error. >>> >>> For me it it 100% irrelevant if "not found" is an error for the query >>> function or not. I just have to be able to check for "not found" and >>> react accordingly. >>> >>> And adding a function >>> >>> def platform_get_irq_opional(): >>> ret =3D platform_get_irq() >>> if ret =3D=3D -ENXIO: >>> return 0 >>> return ret >>> >>> it's not a useful addition to the API if I cannot use 0 as a dummy >>> because it doesn't simplify the caller enough to justify the additional >>> function. >>> >>> The only thing I need to be able is to distinguish the cases "there is >>> an irq", "there is no irq" and anything else is "there is a problem I >>> cannot handle and so forward it to my caller". The semantic of >>> platform_get_irq() is able to satisfy this requirement[1], so why introdu= >> ce >>> platform_get_irq_opional() for the small advantage that I can check for >>> not-found using >>> >>> if (!irq) >>> >>> instead of >>> >>> if (irq !=3D -ENXIO) >>> >>> ? The semantic of platform_get_irq() is easier ("Either a usable >>> non-negative irq number or a negative error number") compared to >>> platform_get_irq_optional() ("Either a usable positive irq number or a >>> negative error number or 0 meaning not found"). Usage of >>> platform_get_irq() isn't harder or more expensive (neither for a human >>> reader nor for a maching running the resulting compiled code). >>> For a human reader >>> >>> if (irq !=3D -ENXIO) >>> >>> is even easier to understand because for >>> >>> if (!irq) >>> >>> they have to check where the value comes from, see it's >>> platform_get_irq_optional() and understand that 0 means not-found. >> >> "vIRQ zero does not exist." > > With that statement in mind I would expect that a function that gives me > an (v)irq number never returns 0. > >>> This function just adds overhead because as a irq framework user I have >>> to understand another function. For me the added benefit is too small to >>> justify the additional function. And you break out-of-tree drivers. >>> These are all no major counter arguments, but as the advantage isn't >>> major either, they still matter. >>> >>> Best regards >>> Uwe >>> >>> [1] the only annoying thing is the error message. >> >> So there's still a need for two functions. > > Or a single function not emitting an error message together with the > callers being responsible for calling dev_err(). > > So the options in my preference order (first is best) are: > > - Remove the printk from platform_get_irq() and remove > platform_get_irq_optional(); Strong NAK here: - dev_err() in our function saves a lot of (repeatable!) comments; - we've already discussed that it's more optimal to check againt 0 than against -ENXIO in the callers. > - Rename platform_get_irq_optional() to platform_get_irq_silently() NAK as well. We'd better off complaining about irq < 0 in this function. > - Keep platform_get_irq_optional() as is NAK, it's suboptimal in the call sites. > - Collect underpants > > - ? You're on your own here. :-) > - Change semantic of platform_get_irq_optional() Yes, we should change the semantics if it serves our goals better. > Best regards > Uwe MBR, Sergey
On 1/19/22 7:12 PM, Sergey Shtylyov wrote: [...] >>> So there's still a need for two functions. >> >> Or a single function not emitting an error message together with the >> callers being responsible for calling dev_err(). >> >> So the options in my preference order (first is best) are: >> >> - Remove the printk from platform_get_irq() and remove >> platform_get_irq_optional(); > > Strong NAK here: > - dev_err() in our function saves a lot of (repeatable!) comments; s/comments/code/. Actually, I think I can accept the removal of dev_err_probe() in platform_get_irq() as this is not a common practice anyway (yet? :-))... > - we've already discussed that it's more optimal to check againt 0 than Against. :-) > against -ENXIO in the callers. And we also aim to be the error code agnostic in the callers... >> - Rename platform_get_irq_optional() to platform_get_irq_silently() > > NAK as well. We'd better off complaining about irq < 0 in this function. >> - Keep platform_get_irq_optional() as is > > NAK, it's suboptimal in the call sites. s/in/on/. [...] >> Best regards >> Uwe MBR, Sergey
On Mon, Jan 17, 2022 at 09:47:32AM +0100, Uwe Kleine-König wrote: > On Sun, Jan 16, 2022 at 09:15:20PM +0300, Sergey Shtylyov wrote: ... > 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. It's what we are against of. The idea is to have irq = platform_get_irq_optional(...) if (irq < 0) // we do not care about special cookies here return irq; if (irq) setup_irq(irq) else setup_polling() See the difference? Your code is convoluted. > 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.
On Sat, Jan 15, 2022 at 07:36:43PM +0100, Uwe Kleine-König wrote: > Hello, > > I'm trying to objectively summarize the discussions in this thread in > the hope this helps finding a way that most people can live with. > > First a description of the status quo: I do not really understand why we put an equal sign in all implications between meaning of the 0 cookie and NULL as an (non-existed) instance of an object? It's like comparing None object in Python to False.
On 1/19/22 9:58 PM, Andy Shevchenko wrote: [...] >> 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. > > It's what we are against of. The idea is to have > > irq = platform_get_irq_optional(...) > if (irq < 0) // we do not care about special cookies here > return irq; > > if (irq) > setup_irq(irq) > else > setup_polling() > > See the difference? Your code is convoluted. ... and it's longer when you look at the translated code! :-) [...] MBR, Sergey
On 1/17/22 4:08 PM, Geert Uytterhoeven wrote: [...] >>> 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. ACK for using dev_err_probe() in platfrom_get_irq_optional() for the real errors... I've also noted that only platfrom_get_irq_optional() got converted from dev_err() to dev_err_probe() but not platfrom_get_irq_byname_optional()... [...] > Gr{oetje,eeting}s, > > Geert MBR, Sergey
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", ®)) { 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 {
This patch is based on the former Andy Shevchenko's patch: https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/ 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 <matthias.schiffer@ew.tq-group.com> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> --- 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(-)