Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved

From: Sergei Shtylyov <sergei.shtylyov@gmail.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Sergey Shtylyov <s.shtylyov@omp.ru>
Cc: Damien Le Moal <damien.lemoal@opensource.wdc.com>,
	linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org,
	Hans de Goede <hdegoede@redhat.com>, Jens Axboe <axboe@kernel.dk>
Subject: Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved
Date: Fri, 10 Dec 2021 20:15:43 +0300	[thread overview]
Message-ID: <6c03ffef-b2e0-16ba-35f3-206af2a611d2@gmail.com> (raw)
In-Reply-To: <YbM7xkTazM76CVvD@smile.fi.intel.com>

On 12/10/21 2:36 PM, Andy Shevchenko wrote:

>>>>>> platform_get_irq() will print a message when it fails.
>>>>>> No need to repeat this.
>>>>>>
>>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills
>>>>>> out a big WARN() in such case.
>>>>>
>>>>> The reason you should be able to remove the "if (!irq)" test is that
>>>>> platform_get_irq() never returns 0. At least, that is what the function kdoc
>>>>> says. But looking at platform_get_irq_optional(), which is called by
>>>>> platform_get_irq(), the out label is:
>>>>>
>>>>> 	WARN(ret == 0, "0 is an invalid IRQ number\n");
>>>>> 	return ret;
>>>>>
>>>>> So 0 will be returned as-is. That is rather weird. That should be fixed to
>>>>> return -ENXIO:
>>>>>
>>>>> 	if (WARN(ret == 0, "0 is an invalid IRQ number\n"))
>>>>> 		return -ENXIO;
>>>>> 	return ret;
>>>>
>>>>    My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this
>>>> but returns -EINVAL instead.
>>>>
>>>>> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ?
>>>>
>>>>    Of course it isn't...
>>>
>>> It's unsubstantiated statement. The vIRQ 0 shouldn't be returned by any of
>>> those API calls.
>>
>>    We do _not_ know what needs to be fixed, that's the problem, and that's why the WARN()
>> is there...
> 
> So, have you seen this warning (being reported) related to libahci_platform?

   No (as if you need to really see this while it's obvious from the code review).

> If no, what we are discussing about then? The workaround is redundant and

   I don't know. :-) Your arguments so far seem bogus (sorry! :-))...

> no need to have a dead code in the driver, really.

  "Jazz isn't dead, it just smells funny". :-)

>>> If it is the case, go and fix them, no need to workaround
>>> in each of the callers.
>>
>>    There's a need to work around as long as IRQ0 ican be returned, otherwise
>>    we get partly functioning or non-functioning drivers...
> 
> You get them unfunctioning anyways

   The drivers would be broken in not quite obvious ways. With IRQ0 check, they just
don't probe anymore. See the explanation of the IRQ0 check (in the drivers) in my
previous mail...

> and you get the big WARN() even before this patch.

   As if that was enough...
   The IRQ0 problem exists for at least 15 (if not 20) years...

MBR, Sergey