| Message ID | 20191019094528.27850-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm: Switch to platform_get_irq_optional() | expand |
On Sat, Oct 19, 2019 at 11:45:28AM +0200, Hans de Goede wrote: > Since commit 7723f4c5ecdb ("driver core: platform: Add an error message to > platform_get_irq*()"), platform_get_irq() will call dev_err() on an error, > as the IRQ usage in the tpm_tis driver is optional, this is undesirable. This should have a fixes line for the above, or maybe the commit that addtion the _optional version.. Jason
On Sat, Oct 19, 2019 at 11:45:28AM +0200, Hans de Goede wrote: > Since commit 7723f4c5ecdb ("driver core: platform: Add an error message to > platform_get_irq*()"), platform_get_irq() will call dev_err() on an error, > as the IRQ usage in the tpm_tis driver is optional, this is undesirable. > > Specifically this leads to this new false-positive error being logged: > [ 5.135413] tpm_tis MSFT0101:00: IRQ index 0 not found > > This commit switches to platform_get_irq_optional(), which does not log > an error, fixing this. > > Cc: <stable@vger.kernel.org> # 5.4.x Incorrect format (should be wo '<' and '>'). Also, not sure why this should be backported to stable kernel anyway. /Jarkko
Hi, On 21-10-2019 17:49, Jarkko Sakkinen wrote: > On Sat, Oct 19, 2019 at 11:45:28AM +0200, Hans de Goede wrote: >> Since commit 7723f4c5ecdb ("driver core: platform: Add an error message to >> platform_get_irq*()"), platform_get_irq() will call dev_err() on an error, >> as the IRQ usage in the tpm_tis driver is optional, this is undesirable. >> >> Specifically this leads to this new false-positive error being logged: >> [ 5.135413] tpm_tis MSFT0101:00: IRQ index 0 not found >> >> This commit switches to platform_get_irq_optional(), which does not log >> an error, fixing this. >> >> Cc: <stable@vger.kernel.org> # 5.4.x > > Incorrect format (should be wo '<' and '>'). According to: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html the '<' and '>' should be added when adding a # <kerner> > Also, not sure why this should be backported to stable kernel anyway. Because false-positive error messages are bad and cause users to file false-positive bug-reports. Regards, Hans
On Mon, Oct 21, 2019 at 11:05:02AM -0300, Jason Gunthorpe wrote: > On Sat, Oct 19, 2019 at 11:45:28AM +0200, Hans de Goede wrote: > > Since commit 7723f4c5ecdb ("driver core: platform: Add an error message to > > platform_get_irq*()"), platform_get_irq() will call dev_err() on an error, > > as the IRQ usage in the tpm_tis driver is optional, this is undesirable. > > This should have a fixes line for the above, or maybe the commit that > addtion the _optional version.. Is this fixing something? /Jarkko
On Mon, Oct 21, 2019 at 05:56:56PM +0200, Hans de Goede wrote: > Hi, > > On 21-10-2019 17:49, Jarkko Sakkinen wrote: > > On Sat, Oct 19, 2019 at 11:45:28AM +0200, Hans de Goede wrote: > > > Since commit 7723f4c5ecdb ("driver core: platform: Add an error message to > > > platform_get_irq*()"), platform_get_irq() will call dev_err() on an error, > > > as the IRQ usage in the tpm_tis driver is optional, this is undesirable. > > > > > > Specifically this leads to this new false-positive error being logged: > > > [ 5.135413] tpm_tis MSFT0101:00: IRQ index 0 not found > > > > > > This commit switches to platform_get_irq_optional(), which does not log > > > an error, fixing this. > > > > > > Cc: <stable@vger.kernel.org> # 5.4.x > > > > Incorrect format (should be wo '<' and '>'). > > According to: > > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html > > the '<' and '>' should be added when adding a # <kerner> OK, right so it was. This first patch that I'm reviewing with such commit. > > Also, not sure why this should be backported to stable kernel anyway. > > Because false-positive error messages are bad and cause users to > file false-positive bug-reports. Neither categorizes into a regression albeit being unfortunate glitches. /Jarkko
Hi, On 23-10-2019 13:37, Jarkko Sakkinen wrote: > On Mon, Oct 21, 2019 at 05:56:56PM +0200, Hans de Goede wrote: >> Hi, >> >> On 21-10-2019 17:49, Jarkko Sakkinen wrote: >>> On Sat, Oct 19, 2019 at 11:45:28AM +0200, Hans de Goede wrote: >>>> Since commit 7723f4c5ecdb ("driver core: platform: Add an error message to >>>> platform_get_irq*()"), platform_get_irq() will call dev_err() on an error, >>>> as the IRQ usage in the tpm_tis driver is optional, this is undesirable. >>>> >>>> Specifically this leads to this new false-positive error being logged: >>>> [ 5.135413] tpm_tis MSFT0101:00: IRQ index 0 not found >>>> >>>> This commit switches to platform_get_irq_optional(), which does not log >>>> an error, fixing this. >>>> >>>> Cc: <stable@vger.kernel.org> # 5.4.x >>> >>> Incorrect format (should be wo '<' and '>'). >> >> According to: >> >> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html >> >> the '<' and '>' should be added when adding a # <kerner> > > OK, right so it was. This first patch that I'm reviewing with such > commit. > >>> Also, not sure why this should be backported to stable kernel anyway. >> >> Because false-positive error messages are bad and cause users to >> file false-positive bug-reports. > > Neither categorizes into a regression albeit being unfortunate > glitches. The stable series also does other small fixes, such as adding new pci/usb-ids, etc. This clearly falls within this. IMHO ideally this should go to a 5.4-rc# making the whole discussion moot, but since I was afraid it would not make 5.4, I added the Cc: stable. Regards, Hans
On Wed, Oct 23, 2019 at 04:32:57PM +0200, Hans de Goede wrote: > Hi, > > On 23-10-2019 13:37, Jarkko Sakkinen wrote: > > On Mon, Oct 21, 2019 at 05:56:56PM +0200, Hans de Goede wrote: > > > Hi, > > > > > > On 21-10-2019 17:49, Jarkko Sakkinen wrote: > > > > On Sat, Oct 19, 2019 at 11:45:28AM +0200, Hans de Goede wrote: > > > > > Since commit 7723f4c5ecdb ("driver core: platform: Add an error message to > > > > > platform_get_irq*()"), platform_get_irq() will call dev_err() on an error, > > > > > as the IRQ usage in the tpm_tis driver is optional, this is undesirable. > > > > > > > > > > Specifically this leads to this new false-positive error being logged: > > > > > [ 5.135413] tpm_tis MSFT0101:00: IRQ index 0 not found > > > > > > > > > > This commit switches to platform_get_irq_optional(), which does not log > > > > > an error, fixing this. > > > > > > > > > > Cc: <stable@vger.kernel.org> # 5.4.x > > > > > > > > Incorrect format (should be wo '<' and '>'). > > > > > > According to: > > > > > > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html > > > > > > the '<' and '>' should be added when adding a # <kerner> > > > > OK, right so it was. This first patch that I'm reviewing with such > > commit. > > > > > > Also, not sure why this should be backported to stable kernel anyway. > > > > > > Because false-positive error messages are bad and cause users to > > > file false-positive bug-reports. > > > > Neither categorizes into a regression albeit being unfortunate > > glitches. > > The stable series also does other small fixes, such as adding new > pci/usb-ids, etc. This clearly falls within this. IMHO ideally this > should go to a 5.4-rc# making the whole discussion moot, but since > I was afraid it would not make 5.4, I added the Cc: stable. I get adding PCI/USB id as it extends the hardware support for the stable kernel without risking its stability. This patch is factors less useful. /Jarkko
Hi, On 24-10-2019 16:25, Jarkko Sakkinen wrote: > On Wed, Oct 23, 2019 at 04:32:57PM +0200, Hans de Goede wrote: >> Hi, >> >> On 23-10-2019 13:37, Jarkko Sakkinen wrote: >>> On Mon, Oct 21, 2019 at 05:56:56PM +0200, Hans de Goede wrote: >>>> Hi, >>>> >>>> On 21-10-2019 17:49, Jarkko Sakkinen wrote: >>>>> On Sat, Oct 19, 2019 at 11:45:28AM +0200, Hans de Goede wrote: >>>>>> Since commit 7723f4c5ecdb ("driver core: platform: Add an error message to >>>>>> platform_get_irq*()"), platform_get_irq() will call dev_err() on an error, >>>>>> as the IRQ usage in the tpm_tis driver is optional, this is undesirable. >>>>>> >>>>>> Specifically this leads to this new false-positive error being logged: >>>>>> [ 5.135413] tpm_tis MSFT0101:00: IRQ index 0 not found >>>>>> >>>>>> This commit switches to platform_get_irq_optional(), which does not log >>>>>> an error, fixing this. >>>>>> >>>>>> Cc: <stable@vger.kernel.org> # 5.4.x >>>>> >>>>> Incorrect format (should be wo '<' and '>'). >>>> >>>> According to: >>>> >>>> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html >>>> >>>> the '<' and '>' should be added when adding a # <kerner> >>> >>> OK, right so it was. This first patch that I'm reviewing with such >>> commit. >>> >>>>> Also, not sure why this should be backported to stable kernel anyway. >>>> >>>> Because false-positive error messages are bad and cause users to >>>> file false-positive bug-reports. >>> >>> Neither categorizes into a regression albeit being unfortunate >>> glitches. >> >> The stable series also does other small fixes, such as adding new >> pci/usb-ids, etc. This clearly falls within this. IMHO ideally this >> should go to a 5.4-rc# making the whole discussion moot, but since >> I was afraid it would not make 5.4, I added the Cc: stable. > > I get adding PCI/USB id as it extends the hardware support for the > stable kernel without risking its stability. This patch is factors > less useful. It also has about 0% chance of causing regressions and it does help to avoid false--positive bug reports. TBH I'm quite surprised we are even having this discussion... Regards, Hans
On Wed, Oct 23, 2019 at 02:32:48PM +0300, Jarkko Sakkinen wrote: > On Mon, Oct 21, 2019 at 11:05:02AM -0300, Jason Gunthorpe wrote: > > On Sat, Oct 19, 2019 at 11:45:28AM +0200, Hans de Goede wrote: > > > Since commit 7723f4c5ecdb ("driver core: platform: Add an error message to > > > platform_get_irq*()"), platform_get_irq() will call dev_err() on an error, > > > as the IRQ usage in the tpm_tis driver is optional, this is undesirable. > > > > This should have a fixes line for the above, or maybe the commit that > > addtion the _optional version.. > > Is this fixing something? Yes, an earlier commit caused new bogus warnings to appear, this is fixing that regression Jason
On Thu, Oct 24, 2019 at 04:27:24PM +0200, Hans de Goede wrote: > Hi, > > On 24-10-2019 16:25, Jarkko Sakkinen wrote: > > On Wed, Oct 23, 2019 at 04:32:57PM +0200, Hans de Goede wrote: > > > Hi, > > > > > > On 23-10-2019 13:37, Jarkko Sakkinen wrote: > > > > On Mon, Oct 21, 2019 at 05:56:56PM +0200, Hans de Goede wrote: > > > > > Hi, > > > > > > > > > > On 21-10-2019 17:49, Jarkko Sakkinen wrote: > > > > > > On Sat, Oct 19, 2019 at 11:45:28AM +0200, Hans de Goede wrote: > > > > > > > Since commit 7723f4c5ecdb ("driver core: platform: Add an error message to > > > > > > > platform_get_irq*()"), platform_get_irq() will call dev_err() on an error, > > > > > > > as the IRQ usage in the tpm_tis driver is optional, this is undesirable. > > > > > > > > > > > > > > Specifically this leads to this new false-positive error being logged: > > > > > > > [ 5.135413] tpm_tis MSFT0101:00: IRQ index 0 not found > > > > > > > > > > > > > > This commit switches to platform_get_irq_optional(), which does not log > > > > > > > an error, fixing this. > > > > > > > > > > > > > > Cc: <stable@vger.kernel.org> # 5.4.x > > > > > > > > > > > > Incorrect format (should be wo '<' and '>'). > > > > > > > > > > According to: > > > > > > > > > > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html > > > > > > > > > > the '<' and '>' should be added when adding a # <kerner> > > > > > > > > OK, right so it was. This first patch that I'm reviewing with such > > > > commit. > > > > > > > > > > Also, not sure why this should be backported to stable kernel anyway. > > > > > > > > > > Because false-positive error messages are bad and cause users to > > > > > file false-positive bug-reports. > > > > > > > > Neither categorizes into a regression albeit being unfortunate > > > > glitches. > > > > > > The stable series also does other small fixes, such as adding new > > > pci/usb-ids, etc. This clearly falls within this. IMHO ideally this > > > should go to a 5.4-rc# making the whole discussion moot, but since > > > I was afraid it would not make 5.4, I added the Cc: stable. > > > > I get adding PCI/USB id as it extends the hardware support for the > > stable kernel without risking its stability. This patch is factors > > less useful. > > It also has about 0% chance of causing regressions and it does > help to avoid false--positive bug reports. > > TBH I'm quite surprised we are even having this discussion... > > Regards, > > Hans Why do you think that way? I mean the commit does not even have a fixes line. It already obviously implies that this kind of discussion is mandatory. Your reasoning in this discussion does make sense. The problem is really the commit message supplied. I'd guess something like this would be more appropriate: " platform_get_irq() calls dev_err() on an error. As the IRQ usage in the tpm_tis driver is optional, this is undesirable. Specifically this leads to this new false-positive error being logged: [ 5.135413] tpm_tis MSFT0101:00: IRQ index 0 not found This commit switches to platform_get_irq_optional(), which does not log an error, fixing this. Fixes: 7723f4c5ecdb ("driver core: platform: Add an error message to platform_get_irq*()" Cc: <stable@vger.kernel.org> # 5.4.x Signed-off-by: Hans de Goede <hdegoede@redhat.com> " You should confirm this by sending v2 either what I proposed unmodified or with the changes that you see appropriate. /Jarkko
On Thu, Oct 24, 2019 at 10:09:42PM +0300, Jarkko Sakkinen wrote: > Why do you think that way? > > I mean the commit does not even have a fixes line. It already obviously > implies that this kind of discussion is mandatory. Your reasoning in > this discussion does make sense. The problem is really the commit > message supplied. > > I'd guess something like this would be more appropriate: > > " > platform_get_irq() calls dev_err() on an error. As the IRQ usage in the > tpm_tis driver is optional, this is undesirable. > > Specifically this leads to this new false-positive error being logged: > [ 5.135413] tpm_tis MSFT0101:00: IRQ index 0 not found > > This commit switches to platform_get_irq_optional(), which does not log > an error, fixing this. > > Fixes: 7723f4c5ecdb ("driver core: platform: Add an error message to platform_get_irq*()" > Cc: <stable@vger.kernel.org> # 5.4.x > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > " This is much better, yes Jason
Hi, On 24-10-2019 21:09, Jarkko Sakkinen wrote: > On Thu, Oct 24, 2019 at 04:27:24PM +0200, Hans de Goede wrote: >> Hi, >> >> On 24-10-2019 16:25, Jarkko Sakkinen wrote: >>> On Wed, Oct 23, 2019 at 04:32:57PM +0200, Hans de Goede wrote: >>>> Hi, >>>> >>>> On 23-10-2019 13:37, Jarkko Sakkinen wrote: >>>>> On Mon, Oct 21, 2019 at 05:56:56PM +0200, Hans de Goede wrote: >>>>>> Hi, >>>>>> >>>>>> On 21-10-2019 17:49, Jarkko Sakkinen wrote: >>>>>>> On Sat, Oct 19, 2019 at 11:45:28AM +0200, Hans de Goede wrote: >>>>>>>> Since commit 7723f4c5ecdb ("driver core: platform: Add an error message to >>>>>>>> platform_get_irq*()"), platform_get_irq() will call dev_err() on an error, >>>>>>>> as the IRQ usage in the tpm_tis driver is optional, this is undesirable. >>>>>>>> >>>>>>>> Specifically this leads to this new false-positive error being logged: >>>>>>>> [ 5.135413] tpm_tis MSFT0101:00: IRQ index 0 not found >>>>>>>> >>>>>>>> This commit switches to platform_get_irq_optional(), which does not log >>>>>>>> an error, fixing this. >>>>>>>> >>>>>>>> Cc: <stable@vger.kernel.org> # 5.4.x >>>>>>> >>>>>>> Incorrect format (should be wo '<' and '>'). >>>>>> >>>>>> According to: >>>>>> >>>>>> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html >>>>>> >>>>>> the '<' and '>' should be added when adding a # <kerner> >>>>> >>>>> OK, right so it was. This first patch that I'm reviewing with such >>>>> commit. >>>>> >>>>>>> Also, not sure why this should be backported to stable kernel anyway. >>>>>> >>>>>> Because false-positive error messages are bad and cause users to >>>>>> file false-positive bug-reports. >>>>> >>>>> Neither categorizes into a regression albeit being unfortunate >>>>> glitches. >>>> >>>> The stable series also does other small fixes, such as adding new >>>> pci/usb-ids, etc. This clearly falls within this. IMHO ideally this >>>> should go to a 5.4-rc# making the whole discussion moot, but since >>>> I was afraid it would not make 5.4, I added the Cc: stable. >>> >>> I get adding PCI/USB id as it extends the hardware support for the >>> stable kernel without risking its stability. This patch is factors >>> less useful. >> >> It also has about 0% chance of causing regressions and it does >> help to avoid false--positive bug reports. >> >> TBH I'm quite surprised we are even having this discussion... >> >> Regards, >> >> Hans > > Why do you think that way? > > I mean the commit does not even have a fixes line. It already obviously > implies that this kind of discussion is mandatory. Your reasoning in > this discussion does make sense. The problem is really the commit > message supplied. > > I'd guess something like this would be more appropriate: > > " > platform_get_irq() calls dev_err() on an error. As the IRQ usage in the > tpm_tis driver is optional, this is undesirable. > > Specifically this leads to this new false-positive error being logged: > [ 5.135413] tpm_tis MSFT0101:00: IRQ index 0 not found > > This commit switches to platform_get_irq_optional(), which does not log > an error, fixing this. > > Fixes: 7723f4c5ecdb ("driver core: platform: Add an error message to platform_get_irq*()" > Cc: <stable@vger.kernel.org> # 5.4.x > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > " > > You should confirm this by sending v2 either what I proposed unmodified > or with the changes that you see appropriate. Ok, this is fine with me, I will send v2 with the updated commit msg right away. Regards, Hans
On Fri, Oct 25, 2019 at 11:13:21AM +0200, Hans de Goede wrote:
> Ok, this is fine with me, I will send v2 with the updated commit msg right away.
Thank you, appreciate it.
/Jarkko
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index e4fdde93ed4c..e7df342a317d 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -286,7 +286,7 @@ static int tpm_tis_plat_probe(struct platform_device *pdev) } tpm_info.res = *res; - tpm_info.irq = platform_get_irq(pdev, 0); + tpm_info.irq = platform_get_irq_optional(pdev, 0); if (tpm_info.irq <= 0) { if (pdev != force_pdev) tpm_info.irq = -1;
Since commit 7723f4c5ecdb ("driver core: platform: Add an error message to platform_get_irq*()"), platform_get_irq() will call dev_err() on an error, as the IRQ usage in the tpm_tis driver is optional, this is undesirable. Specifically this leads to this new false-positive error being logged: [ 5.135413] tpm_tis MSFT0101:00: IRQ index 0 not found This commit switches to platform_get_irq_optional(), which does not log an error, fixing this. Cc: <stable@vger.kernel.org> # 5.4.x Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/char/tpm/tpm_tis.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)