| Message ID | 20240617135338.82006-1-simont@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3ec1428d7b7c519d757a013cef908d7e33dee882 |
| Headers | show |
| Series | ASoC: cs35l56: Accept values greater than 0 as IRQ numbers | expand |
On Mon, Jun 17, 2024 at 02:53:38PM +0100, Simon Trimmer wrote: > IRQ lookup functions such as those in ACPI can return error values when > an IRQ is not defined. The i2c core driver converts the error codes to a > value of 0 and the SPI bus driver passes them unaltered to client device > drivers. > > The cs35l56 driver should only accept positive non-zero values as IRQ > numbers. Have all architectures removed 0 as a valid IRQ?
On 17/06/2024 15:04, Mark Brown wrote: > On Mon, Jun 17, 2024 at 02:53:38PM +0100, Simon Trimmer wrote: >> IRQ lookup functions such as those in ACPI can return error values when >> an IRQ is not defined. The i2c core driver converts the error codes to a >> value of 0 and the SPI bus driver passes them unaltered to client device >> drivers. >> >> The cs35l56 driver should only accept positive non-zero values as IRQ >> numbers. > > Have all architectures removed 0 as a valid IRQ? From discussion threads we can find 0 might still used on x86 for a legacy device. But the conversations we can find on this don't seem to exclude passing a negative error number, just that 0 can normally be assumed invalid. The kerneldoc for SPI says: * @irq: Negative, or the number passed to request_irq() to receive * interrupts from this device.
> From: Richard Fitzgerald <rf@opensource.cirrus.com> > Sent: Monday, June 17, 2024 3:34 PM > On 17/06/2024 15:04, Mark Brown wrote: > > On Mon, Jun 17, 2024 at 02:53:38PM +0100, Simon Trimmer wrote: > >> IRQ lookup functions such as those in ACPI can return error values when > >> an IRQ is not defined. The i2c core driver converts the error codes to a > >> value of 0 and the SPI bus driver passes them unaltered to client device > >> drivers. > >> > >> The cs35l56 driver should only accept positive non-zero values as IRQ > >> numbers. > > > > Have all architectures removed 0 as a valid IRQ? > > From discussion threads we can find 0 might still used on x86 for a > legacy device. > But the conversations we can find on this don't seem to exclude passing > a negative error number, just that 0 can normally be assumed invalid. > > The kerneldoc for SPI says: > > * @irq: Negative, or the number passed to request_irq() to receive > * interrupts from this device. Yes and the threads of these lore links in these commits are rather feisty ce753ad1549c platform: finally disallow IRQ0 in platform_get_irq() and its ilk a85a6c86c25b driver core: platform: Clarify that IRQ 0 is invalid
On 17/06/2024 15:48, Simon Trimmer wrote: >> From: Richard Fitzgerald <rf@opensource.cirrus.com> >> Sent: Monday, June 17, 2024 3:34 PM >> On 17/06/2024 15:04, Mark Brown wrote: >>> On Mon, Jun 17, 2024 at 02:53:38PM +0100, Simon Trimmer wrote: >>>> IRQ lookup functions such as those in ACPI can return error values when >>>> an IRQ is not defined. The i2c core driver converts the error codes to > a >>>> value of 0 and the SPI bus driver passes them unaltered to client > device >>>> drivers. >>>> >>>> The cs35l56 driver should only accept positive non-zero values as IRQ >>>> numbers. >>> >>> Have all architectures removed 0 as a valid IRQ? >> >> From discussion threads we can find 0 might still used on x86 for a >> legacy device. >> But the conversations we can find on this don't seem to exclude passing >> a negative error number, just that 0 can normally be assumed invalid. >> >> The kerneldoc for SPI says: >> >> * @irq: Negative, or the number passed to request_irq() to receive >> * interrupts from this device. > > Yes and the threads of these lore links in these commits are rather feisty > > ce753ad1549c platform: finally disallow IRQ0 in platform_get_irq() and its > ilk > a85a6c86c25b driver core: platform: Clarify that IRQ 0 is invalid > > So 0 is invalid. Question is: is it also valid to pass -ve errors, or is 0 the _only_ invalid value?
On Mon, Jun 17, 2024 at 03:33:59PM +0100, Richard Fitzgerald wrote: > On 17/06/2024 15:04, Mark Brown wrote: > > Have all architectures removed 0 as a valid IRQ? > From discussion threads we can find 0 might still used on x86 for a > legacy device. Some of the arm platforms were also an issue in the past, though possibly they've all been modernised by now. Don't know about other older architectures. > But the conversations we can find on this don't seem to exclude passing > a negative error number, just that 0 can normally be assumed invalid. Yes, the question was specifically about the assumption that 0 is invalid. The status of 0 is kind of a mess, people keep assuming that it isn't valid and it just depends if users of platforms which try to use 0 trip up over it. Sometimes people work on trying to eliminate uses of 0 but it tends to get you into older code nobody wants to touch. > The kerneldoc for SPI says: > * @irq: Negative, or the number passed to request_irq() to receive > * interrupts from this device. Which includes the 0 as valid thing...
On Mon, Jun 17, 2024 at 03:54:04PM +0100, Richard Fitzgerald wrote: > So 0 is invalid. Question is: is it also valid to pass -ve errors, or is > 0 the _only_ invalid value? Negative values should be fine.
On 18/06/2024 17:00, Mark Brown wrote: > On Mon, Jun 17, 2024 at 03:54:04PM +0100, Richard Fitzgerald wrote: > >> So 0 is invalid. Question is: is it also valid to pass -ve errors, or is >> 0 the _only_ invalid value? > > Negative values should be fine. In that case this patch is necessary so we reject negative values as not an IRQ. Otherwise we'll try to request a non-existant IRQ and fail with an error.
On 18/06/2024 16:58, Mark Brown wrote: > On Mon, Jun 17, 2024 at 03:33:59PM +0100, Richard Fitzgerald wrote: >> On 17/06/2024 15:04, Mark Brown wrote: > >>> Have all architectures removed 0 as a valid IRQ? > >> From discussion threads we can find 0 might still used on x86 for a >> legacy device. > > Some of the arm platforms were also an issue in the past, though > possibly they've all been modernised by now. Don't know about other > older architectures. > >> But the conversations we can find on this don't seem to exclude passing >> a negative error number, just that 0 can normally be assumed invalid. > > Yes, the question was specifically about the assumption that 0 is > invalid. The status of 0 is kind of a mess, people keep assuming that > it isn't valid and it just depends if users of platforms which try to > use 0 trip up over it. Sometimes people work on trying to eliminate > uses of 0 but it tends to get you into older code nobody wants to touch. > >> The kerneldoc for SPI says: > >> * @irq: Negative, or the number passed to request_irq() to receive >> * interrupts from this device. > > Which includes the 0 as valid thing... The statement of truth from Linus Torvalds et al. seems to be that 0 is invalid except on x86. And on x86 it is specifically reserved for a legacy timer IRQ so it can't be anything else.
On 17/06/2024 14:53, Simon Trimmer wrote: > IRQ lookup functions such as those in ACPI can return error values when > an IRQ is not defined. The i2c core driver converts the error codes to a > value of 0 and the SPI bus driver passes them unaltered to client device > drivers. > > The cs35l56 driver should only accept positive non-zero values as IRQ > numbers. > > Signed-off-by: Simon Trimmer <simont@opensource.cirrus.com> > Fixes: 8a731fd37f8b ("ASoC: cs35l56: Move utility functions to shared file") > --- > sound/soc/codecs/cs35l56-shared.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sound/soc/codecs/cs35l56-shared.c b/sound/soc/codecs/cs35l56-shared.c > index 27869e14e9c8..880228f89baf 100644 > --- a/sound/soc/codecs/cs35l56-shared.c > +++ b/sound/soc/codecs/cs35l56-shared.c > @@ -397,7 +397,7 @@ int cs35l56_irq_request(struct cs35l56_base *cs35l56_base, int irq) > { > int ret; > > - if (!irq) > + if (irq < 1) > return 0; > > ret = devm_request_threaded_irq(cs35l56_base->dev, irq, NULL, cs35l56_irq, Mark, I don't understand what your objection is. What is it you want us to do to get this bugfix accepted?
On Wed, Jun 19, 2024 at 10:44:47AM +0100, Richard Fitzgerald wrote: > Mark, I don't understand what your objection is. > What is it you want us to do to get this bugfix accepted? Have patience.
On 19/06/2024 11:22, Mark Brown wrote: > On Wed, Jun 19, 2024 at 10:44:47AM +0100, Richard Fitzgerald wrote: > >> Mark, I don't understand what your objection is. >> What is it you want us to do to get this bugfix accepted? > > Have patience. Ah, ok. Sorry, I assumed you were objecting not just overloaded.
On Wed, Jun 19, 2024 at 11:24:06AM +0100, Richard Fitzgerald wrote:
> Ah, ok. Sorry, I assumed you were objecting not just overloaded.
There's a latency between me deciding to apply a patch and the patch
actually ending up in my tree - I test everything which takes time.
On Mon, 17 Jun 2024 14:53:38 +0100, Simon Trimmer wrote: > IRQ lookup functions such as those in ACPI can return error values when > an IRQ is not defined. The i2c core driver converts the error codes to a > value of 0 and the SPI bus driver passes them unaltered to client device > drivers. > > The cs35l56 driver should only accept positive non-zero values as IRQ > numbers. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: cs35l56: Accept values greater than 0 as IRQ numbers commit: 3ec1428d7b7c519d757a013cef908d7e33dee882 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
diff --git a/sound/soc/codecs/cs35l56-shared.c b/sound/soc/codecs/cs35l56-shared.c index 27869e14e9c8..880228f89baf 100644 --- a/sound/soc/codecs/cs35l56-shared.c +++ b/sound/soc/codecs/cs35l56-shared.c @@ -397,7 +397,7 @@ int cs35l56_irq_request(struct cs35l56_base *cs35l56_base, int irq) { int ret; - if (!irq) + if (irq < 1) return 0; ret = devm_request_threaded_irq(cs35l56_base->dev, irq, NULL, cs35l56_irq,
IRQ lookup functions such as those in ACPI can return error values when an IRQ is not defined. The i2c core driver converts the error codes to a value of 0 and the SPI bus driver passes them unaltered to client device drivers. The cs35l56 driver should only accept positive non-zero values as IRQ numbers. Signed-off-by: Simon Trimmer <simont@opensource.cirrus.com> Fixes: 8a731fd37f8b ("ASoC: cs35l56: Move utility functions to shared file") --- sound/soc/codecs/cs35l56-shared.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)