Date:   Mon, 13 Dec 2021 13:49:20 +0200
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     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
Message-ID: <YbczQAIMK4KBI9w0@smile.fi.intel.com>
References: <20211209145937.77719-1-andriy.shevchenko@linux.intel.com>
 <d91cf14d-c7d8-1c61-9071-102f38e8c924@opensource.wdc.com>
 <febc7f73-929f-d8a6-ea01-5056b9101b46@omp.ru>
 <549c1825-56e6-de9e-e109-77f0d06cfd0f@opensource.wdc.com>
 <5322dafd-86ad-a293-6005-29384cb96cc8@omp.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5322dafd-86ad-a293-6005-29384cb96cc8@omp.ru>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Sat, Dec 11, 2021 at 01:25:20PM +0300, Sergey Shtylyov wrote:
> On 11.12.2021 2:45, Damien Le Moal 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.
> > 
> > Thinking more about this, shouldn't this change go into platform_get_irq()
> > instead of platform_get_irq_optional() ?
> 
>    Why? platform_get_irq() currently just calls platform_get_irq_optional()...
> 
> > The way I see it, I think that the intended behavior for
> > platform_get_irq_optional() is:
> > 1) If have IRQ, return it, always > 0
> > 2) If no IRQ, return 0
> 
>    That does include the IRQ0 case, right?

What IRQ0 case? We do not expect platform APIs ever to handle vIRQ0
(mind letter v) case. Platforms, where it's the case, should handle
it exceptionally on per architecture basis.

> > 3) If error, return < 0
> > no ?
> 
>   I completely agree, I (after thinking a bit) have no issues with that...
> 
> > And for platform_get_irq(), case (2) becomes an error.
> > Is this the intended semantic ?
> 
>    I don't see how it's different from the current behavior. But we can do
> that as well, I just don't see whether it's really better...
> 
> > I am really not sure here as the functions kdoc description and the code do not
> > match. Which one is correct ?
> 
>    It seems both are wrong. :-)

-- 
With Best Regards,
Andy Shevchenko