linux-kernel - Re: [PATCH] iio: adc: ti_am335x

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f2b18c8b-97f9-49c5-a4a1-a3d5c1ae13eb@phytec.de>
Date:   Tue, 19 Sep 2023 10:21:28 +0000
From:   Wadim Egorov <W.Egorov@...tec.de>
To:     Jonathan Cameron <jic23@...nel.org>
CC:     "lars@...afoo.de" <lars@...afoo.de>,
        "robh@...nel.org" <robh@...nel.org>,
        "heiko@...ech.de" <heiko@...ech.de>,
        "mugunthanvnm@...com" <mugunthanvnm@...com>,
        "peter.ujfalusi@...com" <peter.ujfalusi@...com>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "upstream@...ts.phytec.de" <upstream@...ts.phytec.de>,
        "nm@...com" <nm@...com>
Subject: Re: [PATCH] iio: adc: ti_am335x_adc: Make DMAs optional

Hi Jonathan,

Am 17.09.23 um 12:45 schrieb Jonathan Cameron:
> On Thu, 14 Sep 2023 14:13:00 +0200
> Wadim Egorov <w.egorov@...tec.de> wrote:
>
>> DMAs are optional. Even if the DMA request is unsuccessfully,
>> the ADC can still work properly.
>> Make tiadc_request_dma() not fail if we do not provide dmas &
>> dma-names properties.
>>
>> This actually fixes the wrong error handling of the tiadc_request_dma()
>> result where the probing only failed if -EPROPE_DEFER was returned.
>>
>> Fixes: f438b9da75eb ("drivers: iio: ti_am335x_adc: add dma support")
>>
> No line break here.  Fixes tag is part of the main tag block.
>> Signed-off-by: Wadim Egorov <w.egorov@...tec.de>
>
>> ---
>>   drivers/iio/adc/ti_am335x_adc.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
>> index 8db7a01cb5fb..e14aa9254ab1 100644
>> --- a/drivers/iio/adc/ti_am335x_adc.c
>> +++ b/drivers/iio/adc/ti_am335x_adc.c
>> @@ -543,8 +543,11 @@ static int tiadc_request_dma(struct platform_device *pdev,
>>   	if (IS_ERR(dma->chan)) {
>>   		int ret = PTR_ERR(dma->chan);
>>   
>> +		if (ret != -ENODEV)
>> +			return dev_err_probe(&pdev->dev, ret,
>> +					     "RX DMA channel request failed\n");
>>   		dma->chan = NULL;
>> -		return ret;
>> +		return 0;
>>   	}
>>   
>>   	/* RX buffer */
>> @@ -670,7 +673,7 @@ static int tiadc_probe(struct platform_device *pdev)
>>   	platform_set_drvdata(pdev, indio_dev);
>>   
>>   	err = tiadc_request_dma(pdev, adc_dev);
>> -	if (err && err == -EPROBE_DEFER)
>> +	if (err)
> So this looks like a more subtle change than you are describing.
> In the original code, we backed off only if the return was a PROBE_DEFER, otherwise
> we carried on.
>
> Your change seems to make that happen for any non -ENODEV error, including PROBE_DEFER.
> That's fine, but it's not what the description implies.
>
> Whilst tiadc_request_dma will fail today if the dmas etc is not provided, that seems
> like correct behavior to me.  A function requesting dma fails if it isn't available.
> The handling of whether to carry on the job for the caller.

That makes sense, yes. But stm32-adc is doing the same in its dma 
request function.
So I assumed we can do it like that.

>
> So I think it should just be
> 	if (err && err != -EINVAL)
> 		goto err_dma;

We will end up failing if no dmas are configured because the request 
returns -ENODEV.
So I think it needs to be a check for non -ENODEV.

>
> and no change in tiadc_request_dma()
>
> However, the case you describe should have worked find with existing code
> as it wasn't -EPROBE_DEFER, so I don't understand why you were looking at this
> code block in the first place?

Providing wrong dmas in the device tree should've made the driver fail 
to probe.

Regards,
Wadim

>
> Jonathan
>
>
>>   		goto err_dma;
>>   
>>   	return 0;