Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] uio: disable lazy irq disable to avoid double fire
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     linux-kernel@vger.kernel.org
References: <20200521144209.19413-1-thommyj@gmail.com>
 <20200522091444.GA1192483@kroah.com>
From:   Thommy Jakobsson <thommyj@gmail.com>
Message-ID: <6d5b0844-dcaf-902e-466a-1af5ca340b1e@gmail.com>
Date:   Sat, 23 May 2020 11:26:20 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <20200522091444.GA1192483@kroah.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2020-05-22 11:14, Greg KH wrote:
> On Thu, May 21, 2020 at 04:42:09PM +0200, Thommy Jakobsson wrote:
>> +	if (uioinfo->irq) {
> 
> How is this not true at this point in time based on the code above this?
> ->irq should always be set here, right?
It seems to me like there is a path to continue without an IRQ in the
section above, "uioinfo->irq = UIO_IRQ_NONE;", where UIO_IRQ_NONE is
0. Do you agree?

> 
>> +		struct irq_data *irq_data = irq_get_irq_data(uioinfo->irq);
>> +
>> +		/*
>> +		 * If a level interrupt, dont do lazy disable. Otherwise the
>> +		 * irq will fire again since clearing of the actual cause, on
>> +		 * device level, is done in userspace
>> +		 */
>> +		if (!irq_data) {
>> +			dev_err(&pdev->dev, "unable to get irq data\n");
>> +			ret = -ENXIO;
>> +			goto bad1;
> 
> Why is not having this information all of a sudden an error for this
> code?  What could cause that info to not be there?  Existing systems
> without this set would work just fine before this change, and I think
> this breaks them, right?
irq_data should always exists as long as there is an irq descriptor and
I assumed that the descriptors "should" exists at the point when this
code run. So a NULL would either mean something serious and would be
more of a sanity check than anything else, or (more likely) it is the
wrong irq configured.

The "should" comes from that this code path later registers the irq
(devm_uio_register_device->... ->__uio_register_device->...
->request_irq->... ->request_threaded_irq), and when this happen its an
-EINVAL back if there isn't any descriptor from irq_to_desc (which is
the same function as irq_get_data internally uses). So I don't see any
new breaks from this, but I could be wrong so please correct me in that
case. At least it was my intention to not break anything currently
working. Maybe it is better to just do a dev_dbg and let
request_threaded_irq handle the wrong irq case?

> 
>> +		}
>> +		/*
>> +		 * irqd_is_level_type() isn't used since isn't valid unitil
>> +		 * irq is configured.
>> +		 */
>> +		if (irqd_get_trigger_type(irq_data) & IRQ_TYPE_LEVEL_MASK) {
>> +			dev_info(&pdev->dev, "disable lazy unmask\n");
> 
> Why dev_info?  If drivers are working properly, they should be quiet.
> dev_dbg() is fine to use here if you want to debug things to see what is
> happening.
Agreed

BR,
Thommy Jakobsson