Re: [PATCH] tpm: Switch to platform_get_irq_optional()

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

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