linux-kernel - Re: [PATCH v5] i2c: imx: support DMA defer probing

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: <b919e252-6ff4-4e55-9bd6-4d610dda5348@pengutronix.de>
Date: Fri, 20 Dec 2024 08:06:25 +0100
From: Ahmad Fatoum <a.fatoum@...gutronix.de>
To: Carlos Song <carlos.song@....com>,
 Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Andi Shyti <andi.shyti@...nel.org>, Frank Li <frank.li@....com>,
 "kernel@...gutronix.de" <kernel@...gutronix.de>,
 "shawnguo@...nel.org" <shawnguo@...nel.org>,
 "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
 "festevam@...il.com" <festevam@...il.com>,
 "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
 "imx@...ts.linux.dev" <imx@...ts.linux.dev>,
 "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 Clark Wang <xiaoning.wang@....com>
Subject: Re: [PATCH v5] i2c: imx: support DMA defer probing

Hello Carlos,

On 20.12.24 07:58, Carlos Song wrote:
> 
> 
>> -----Original Message-----
>> From: Oleksij Rempel <o.rempel@...gutronix.de>
>> Sent: Friday, December 20, 2024 2:13 PM
>> To: Carlos Song <carlos.song@....com>
>> Cc: Andi Shyti <andi.shyti@...nel.org>; Frank Li <frank.li@....com>;
>> kernel@...gutronix.de; shawnguo@...nel.org; s.hauer@...gutronix.de;
>> festevam@...il.com; linux-i2c@...r.kernel.org; imx@...ts.linux.dev;
>> linux-arm-kernel@...ts.infradead.org; linux-kernel@...r.kernel.org; Clark Wang
>> <xiaoning.wang@....com>; Ahmad Fatoum <a.fatoum@...gutronix.de>
>> Subject: [EXT] Re: [PATCH v5] i2c: imx: support DMA defer probing
>>
>> Caution: This is an external email. Please take care when clicking links or
>> opening attachments. When in doubt, report the message using the 'Report this
>> email' button
>>
>>
>> On Fri, Dec 20, 2024 at 05:59:38AM +0000, Carlos Song wrote:
>>>>> So we make this logic. Anyway we let the I2C controller registered
>>>>> whether
>>>> DMA is available or not(except defer probe).
>>>>> Ignoring ENODEV and EPROBE_DEFER makes it looks like nothing
>>>>> happened if
>>>> DMA is defer probed or not enabled(This is an expected).
>>>>> However we still need i2c DMA status is known when meet an
>>>>> unexpected
>>>> error, so we use dev_err_probe() to print error.
>>>>
>>>> Why dev_err_probe() instead of dev_err()?
>>>>
>>> Hi,
>>> In patch V2 discussion, Marc suggested just return dev_err_probe(),
>>> but I don't accept it so I choose to use dev_err_probe() to print error in V3.[1]
>> In this case, the two APIs have the same function, do you mean dev_err() is more
>> suitable?
>>
>> Yes, dev_err_probe() should be used in combination with return. For
>> example:
>>   return dev_err_probe(...);
>>
>> It will pass the return value on exit of the function and optionally print of the
>> error message if it is not EPROBE_DEFER. Practically it replace commonly used
>> coding pattern:
>>   if (ret == -EPROBE_DEFER) {
>>     return ret;
>>   } else if (ret) {
>>     dev_err(..);
>>     return ret;
>>   }
>>
> Hi,
> 
> Get your good point. I will change my code in V6:
> +       ret = i2c_imx_dma_request(i2c_imx, phy_addr);
> +       if (ret) {
> +               if (ret == -EPROBE_DEFER)
> +                       goto clk_notifier_unregister;
> +               else if (ret == -ENODEV)
> +                       dev_dbg(&pdev->dev, "Only use PIO mode\n");
> +               else
> +                       dev_err(&pdev->dev, "Failed to setup DMA, only use PIO mode\n");
> +       }
> 
> I think this is what you want to see, right?

This loses the information why the error happens (ret). Using dev_err_probe
even if no probe deferral is expected in that branch is perfectly fine
and the kernel-doc even points it out:

  Using this helper in your probe function is totally fine even if @err
  is known to never be -EPROBE_DEFER.

Cheers,
Ahmad

> 
>> --
>> Pengutronix e.K.                           |
>> |
>> Steuerwalder Str. 21                       |
>> http://www.pen/
>> gutronix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C2950266755a
>> 241c00a9208dd20bd5cf2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
>> %7C638702719862691439%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGki
>> OnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ
>> %3D%3D%7C0%7C%7C%7C&sdata=aIuzJP0v5C6HzOCGnCHobK9Llml3thHclTwu
>> CjD13IM%3D&reserved=0  |
>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0
>> |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:
>> +49-5121-206917-5555 |
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |