| Message ID | 20250117171822.775876-1-andre.werner@systec-electronic.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] serial: sc16is7xx: Extend IRQ check for negative values | expand |
On Fri, Jan 17, 2025 at 06:18:22PM +0100, Andre Werner wrote: > Fix the IRQ check to treat the negative values as No IRQ. > > Signed-off-by: Andre Werner <andre.werner@systec-electronic.com> > --- > V2: > There are no changes to the patch itself. The previous patch submission > had a very weird structure within the discussion thread: > https://lkml.org/lkml/2025/1/16/398 > This is simply a new thread opened for better handling. > --- > drivers/tty/serial/sc16is7xx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > index 7b51cdc274fd..560f45ed19ae 100644 > --- a/drivers/tty/serial/sc16is7xx.c > +++ b/drivers/tty/serial/sc16is7xx.c > @@ -1561,7 +1561,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, > /* Always ask for fixed clock rate from a property. */ > device_property_read_u32(dev, "clock-frequency", &uartclk); > > - s->polling = !!irq; > + s->polling = (irq <= 0); > if (s->polling) > dev_dbg(dev, > "No interrupt pin definition, falling back to polling mode\n"); > -- > 2.48.0 > > What commit id does this "fix"? thanks, greg k-h
On 18. 01. 25, 8:34, Greg KH wrote: > On Fri, Jan 17, 2025 at 06:18:22PM +0100, Andre Werner wrote: >> Fix the IRQ check to treat the negative values as No IRQ. >> >> Signed-off-by: Andre Werner <andre.werner@systec-electronic.com> >> --- >> V2: >> There are no changes to the patch itself. The previous patch submission >> had a very weird structure within the discussion thread: >> https://lkml.org/lkml/2025/1/16/398 >> This is simply a new thread opened for better handling. >> --- >> drivers/tty/serial/sc16is7xx.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c >> index 7b51cdc274fd..560f45ed19ae 100644 >> --- a/drivers/tty/serial/sc16is7xx.c >> +++ b/drivers/tty/serial/sc16is7xx.c >> @@ -1561,7 +1561,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, >> /* Always ask for fixed clock rate from a property. */ >> device_property_read_u32(dev, "clock-frequency", &uartclk); >> >> - s->polling = !!irq; >> + s->polling = (irq <= 0); >> if (s->polling) >> dev_dbg(dev, >> "No interrupt pin definition, falling back to polling mode\n"); >> -- >> 2.48.0 >> >> > > What commit id does this "fix"? And yet, it's worth noting (in the commit log) that it actually does not fix any real problem. It's only a sanity check, right?
> -----Original Message----- > Fix the IRQ check to treat the negative values as No IRQ. It seems to me that this is a real fix and needs a Fixes tag. See below. > Signed-off-by: Andre Werner <andre.werner@systec-electronic.com> > --- > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > index 7b51cdc274fd..560f45ed19ae 100644 > --- a/drivers/tty/serial/sc16is7xx.c > +++ b/drivers/tty/serial/sc16is7xx.c > @@ -1561,7 +1561,7 @@ int sc16is7xx_probe(struct device *dev, const struct > sc16is7xx_devtype *devtype, > /* Always ask for fixed clock rate from a property. */ > device_property_read_u32(dev, "clock-frequency", &uartclk); > > - s->polling = !!irq; > + s->polling = (irq <= 0); When irq>=0 these two lines above have a different outcome! irq==0 => !!irq==false <=> (irq<=0)==true irq==1 => !!irq==true <=> (irq<=0)==false > if (s->polling) > dev_dbg(dev, > "No interrupt pin definition, falling back to polling mode\n"); Kind regards, Maarten
Dear Maarten, > > -----Original Message----- > > Fix the IRQ check to treat the negative values as No IRQ. > > It seems to me that this is a real fix and needs a Fixes tag. > See below. > > > Signed-off-by: Andre Werner <andre.werner@systec-electronic.com> > > --- > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > > index 7b51cdc274fd..560f45ed19ae 100644 > > --- a/drivers/tty/serial/sc16is7xx.c > > +++ b/drivers/tty/serial/sc16is7xx.c > > @@ -1561,7 +1561,7 @@ int sc16is7xx_probe(struct device *dev, const struct > > sc16is7xx_devtype *devtype, > > /* Always ask for fixed clock rate from a property. */ > > device_property_read_u32(dev, "clock-frequency", &uartclk); > > > > - s->polling = !!irq; > > + s->polling = (irq <= 0); > > When irq>=0 these two lines above have a different outcome! > irq==0 => !!irq==false <=> (irq<=0)==true > irq==1 => !!irq==true <=> (irq<=0)==false Thanks for the advice. I have not seen this all the time I looked at the code. I accidentally forget to delete the second '!' as I did with the code tested at the embedded device. Thanks for the advice. Should I need to submit this patch again with a Fixup prefix or what needs to be done? > > > if (s->polling) > > dev_dbg(dev, > > "No interrupt pin definition, falling back to polling mode\n"); > > Kind regards, > Maarten > Regards, André
Dear Greg, > On Fri, Jan 17, 2025 at 06:18:22PM +0100, Andre Werner wrote: > > Fix the IRQ check to treat the negative values as No IRQ. > > > > Signed-off-by: Andre Werner <andre.werner@systec-electronic.com> > > --- > > V2: > > There are no changes to the patch itself. The previous patch submission > > had a very weird structure within the discussion thread: > > https://lkml.org/lkml/2025/1/16/398 > > This is simply a new thread opened for better handling. > > --- > > drivers/tty/serial/sc16is7xx.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > > index 7b51cdc274fd..560f45ed19ae 100644 > > --- a/drivers/tty/serial/sc16is7xx.c > > +++ b/drivers/tty/serial/sc16is7xx.c > > @@ -1561,7 +1561,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, > > /* Always ask for fixed clock rate from a property. */ > > device_property_read_u32(dev, "clock-frequency", &uartclk); > > > > - s->polling = !!irq; > > + s->polling = (irq <= 0); > > if (s->polling) > > dev_dbg(dev, > > "No interrupt pin definition, falling back to polling mode\n"); > > -- > > 2.48.0 > > > > > > What commit id does this "fix"? 104c1b9dde9d859dd01bd2d71a2755a2fae43e15 https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?h=tty-next&id=104c1b9dde9d859dd01bd2d71a2755a2fae43e15 > > thanks, > > greg k-h > Regards, André
On Sat, Jan 18, 2025 at 08:28:49PM +0100, Andre Werner wrote: > Dear Greg, > > > On Fri, Jan 17, 2025 at 06:18:22PM +0100, Andre Werner wrote: > > > Fix the IRQ check to treat the negative values as No IRQ. > > > > > > Signed-off-by: Andre Werner <andre.werner@systec-electronic.com> > > > --- > > > V2: > > > There are no changes to the patch itself. The previous patch submission > > > had a very weird structure within the discussion thread: > > > https://lkml.org/lkml/2025/1/16/398 > > > This is simply a new thread opened for better handling. > > > --- > > > drivers/tty/serial/sc16is7xx.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > > > index 7b51cdc274fd..560f45ed19ae 100644 > > > --- a/drivers/tty/serial/sc16is7xx.c > > > +++ b/drivers/tty/serial/sc16is7xx.c > > > @@ -1561,7 +1561,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, > > > /* Always ask for fixed clock rate from a property. */ > > > device_property_read_u32(dev, "clock-frequency", &uartclk); > > > > > > - s->polling = !!irq; > > > + s->polling = (irq <= 0); > > > if (s->polling) > > > dev_dbg(dev, > > > "No interrupt pin definition, falling back to polling mode\n"); > > > -- > > > 2.48.0 > > > > > > > > > > What commit id does this "fix"? > > 104c1b9dde9d859dd01bd2d71a2755a2fae43e15 > > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?h=tty-next&id=104c1b9dde9d859dd01bd2d71a2755a2fae43e15 > Great, then properly document that with a Fixes: tag when you resend this please. thanks, greg k-h
On 18. 01. 25, 18:20, Andre Werner wrote: > Dear Maarten, > >>> -----Original Message----- >>> Fix the IRQ check to treat the negative values as No IRQ. >> >> It seems to me that this is a real fix and needs a Fixes tag. >> See below. >> >>> Signed-off-by: Andre Werner <andre.werner@systec-electronic.com> >>> --- >>> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c >>> index 7b51cdc274fd..560f45ed19ae 100644 >>> --- a/drivers/tty/serial/sc16is7xx.c >>> +++ b/drivers/tty/serial/sc16is7xx.c >>> @@ -1561,7 +1561,7 @@ int sc16is7xx_probe(struct device *dev, const struct >>> sc16is7xx_devtype *devtype, >>> /* Always ask for fixed clock rate from a property. */ >>> device_property_read_u32(dev, "clock-frequency", &uartclk); >>> >>> - s->polling = !!irq; >>> + s->polling = (irq <= 0); >> >> When irq>=0 these two lines above have a different outcome! >> irq==0 => !!irq==false <=> (irq<=0)==true >> irq==1 => !!irq==true <=> (irq<=0)==false > > Thanks for the advice. I have not seen this all the time I looked at the > code. I accidentally forget to delete the second '!' as I did with the code > tested at the embedded device. Thanks for the advice. > > Should I need to submit this patch again with a Fixup prefix or what needs > to be done? Resubmit with complete description on what is broken and when. Incl. the Fixes: tag. The comment from Maarten suggests that it is broken in a completely different way than you describe in the commit log. thanks,
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c index 7b51cdc274fd..560f45ed19ae 100644 --- a/drivers/tty/serial/sc16is7xx.c +++ b/drivers/tty/serial/sc16is7xx.c @@ -1561,7 +1561,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, /* Always ask for fixed clock rate from a property. */ device_property_read_u32(dev, "clock-frequency", &uartclk); - s->polling = !!irq; + s->polling = (irq <= 0); if (s->polling) dev_dbg(dev, "No interrupt pin definition, falling back to polling mode\n");
Fix the IRQ check to treat the negative values as No IRQ. Signed-off-by: Andre Werner <andre.werner@systec-electronic.com> --- V2: There are no changes to the patch itself. The previous patch submission had a very weird structure within the discussion thread: https://lkml.org/lkml/2025/1/16/398 This is simply a new thread opened for better handling. --- drivers/tty/serial/sc16is7xx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)