On 31/07/17 17:56, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Mon, Jul 31, 2017 at 03:59:35PM +0300, Sergei Shtylyov wrote:
>> On 07/31/2017 12:57 PM, Sergei Shtylyov wrote:
>>
>>>>> of_irq_get() may return 0 as well as negative error number on failure,
>>>>> while the driver only checks for the negative values. The driver would then
>>>>> call request_irq(0, ...) in tegra_adma_alloc_chan_resources() and never get
>>>>> valid channel interrupt.
>>>>>
>>>>> Check for 'tdc->irq <= 0' instead and return -ENXIO from the driver's probe
>>>>> iff of_irq_get() returned 0.
>>>>>
>>>>> Fixes: f46b195799b5 ("dmaengine: tegra-adma: Add support for Tegra210 ADMA")
>>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>
>> [...]
>>
>>>>> Index: slave-dma/drivers/dma/tegra210-adma.c
>>>>> ===================================================================
>>>>> --- slave-dma.orig/drivers/dma/tegra210-adma.c
>>>>> +++ slave-dma/drivers/dma/tegra210-adma.c
>>>>> @@ -717,8 +717,8 @@ static int tegra_adma_probe(struct platf
>>>>> tdc->chan_addr = tdma->base_addr + ADMA_CH_REG_OFFSET(i);
>>>>> tdc->irq = of_irq_get(pdev->dev.of_node, i);
>>>>> - if (tdc->irq < 0) {
>>>>> - ret = tdc->irq;
>>>>> + if (tdc->irq <= 0) {
>>>>> + ret = tdc->irq ?: -ENXIO;
>>>>> goto irq_dispose;
>>>>> }
>>>>>
>>>>
>>>> I think in this particular case it would be better to just call
>>>> platform_get_irq(), which already implements the equivalent.
>>
>> Not that platform_get_irq() was no better (if not worse indeed, as it
>> could return 0 both on failure ad success), until I fixed it back in 2016:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e330b9a6bb35dc7097a4f02cb1ae7b6f96df92af
>>
>>>
>>> OK, I'll try looking into that...
>>
>> I've looked at the commit history and no, I don't agree to use
>> platform_get_irq(): my platform_get_irq() fix landed in mainline later than
>> this driver, so fixing this bug in stable kernels would ensue an extra
>> dependency...
>
> Well, surely the platform_get_irq() function should also be fixed in
> stable kernels, so that all callers can be fixed at the same time.
>
> That said, this does fix a bug, and it fixes it in the least invasive
> way, and the damage I'm complaining about is preexisting, so:
>
> Acked-by: Thierry Reding <treding@xxxxxxxxxx>
FWIW ...
Acked-by: Jon Hunter <jonathanh@xxxxxxxxxx>
Thanks!
Jon
--
nvpublic
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
