[PATCH] tpm: Switch to platform_get_irq_optional()

[PATCH] tpm: Switch to platform_get_irq_optional()

[Hans de Goede]

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(-)

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;
-- 
2.23.0

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

[Jason Gunthorpe]

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

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

[Jarkko Sakkinen]

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

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

[Jason Gunthorpe]

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

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

[Jarkko Sakkinen]

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

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

[Hans de Goede]

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

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

[Jarkko Sakkinen]

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

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

[Hans de Goede]

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

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

[Jarkko Sakkinen]

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

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

[Hans de Goede]

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

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

[Jarkko Sakkinen]

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

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

[Jason Gunthorpe]

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

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

[Hans de Goede]

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

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

[Jarkko Sakkinen]

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