| 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
| ||
|
Message-ID:
<AM0PR0402MB3937419BBB58B75FB8F8DE2DE8072@AM0PR0402MB3937.eurprd04.prod.outlook.com>
Date: Fri, 20 Dec 2024 08:06:33 +0000
From: Carlos Song <carlos.song@....com>
To: Oleksij Rempel <o.rempel@...gutronix.de>
CC: Ahmad Fatoum <a.fatoum@...gutronix.de>, 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
> -----Original Message-----
> From: Oleksij Rempel <o.rempel@...gutronix.de>
> Sent: Friday, December 20, 2024 3:46 PM
> To: Carlos Song <carlos.song@....com>
> Cc: Ahmad Fatoum <a.fatoum@...gutronix.de>; 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>
> 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 07:38:47AM +0000, Carlos Song wrote:
> >
> >
> > > -----Original Message-----
> > > From: Oleksij Rempel <o.rempel@...gutronix.de>
> > > Sent: Friday, December 20, 2024 3:35 PM
> > > To: Ahmad Fatoum <a.fatoum@...gutronix.de>
> > > Cc: Carlos Song <carlos.song@....com>; 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>
> > > 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 08:06:25AM +0100, Ahmad Fatoum wrote:
> > > > 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.
> > >
> > > Thank you for the feedback. While I recognize the benefits of
> > > dev_err_probe() for compact and standardized error handling, using
> > > it without returning its result raises a red flag.
> > >
> > > The function's primary purpose is to combine error logging with
> > > returning the error code. If the return value is not used, it can
> > > create confusion and suggests potential oversight or unintended
> > > behavior. This misuse might mislead readers into thinking that the
> > > function always returns at that point, which is not the case here.
> > >
> > > In this scenario, using dev_err() directly is more explicit and
> > > avoids any ambiguity about the control flow or error handling
> > > intent. It keeps the code clear and aligned with its actual behavior.
> > >
> >
> > how about this?
> >
> > + 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 (%d),
> only use PIO mode\n", ret);
> > + }
>
> Please use human readable version of error value. In this case it will
> be:
> dev_err(&pdev->dev, "Failed to setup DMA (%pe), only use PIO mode\n",
> ERR_PTR(ret));
>
Hi, the ret is from i2c_imx_dma_request() and look like that ret has been converted by PTR_ERR,
So the ret error has been human readable version?
static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, dma_addr_t phy_addr)
{
...
dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL);
if (!dma)
----> return -ENOMEM;
dma->chan_tx = dma_request_chan(dev, "tx");
if (IS_ERR(dma->chan_tx)) {
----> ret = PTR_ERR(dma->chan_tx);
if (ret != -ENODEV && ret != -EPROBE_DEFER)
dev_err(dev, "can't request DMA tx channel (%d)\n", ret);
goto fail_al;
}
dma_sconfig.dst_addr = phy_addr +
(IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
dma_sconfig.dst_maxburst = 1;
dma_sconfig.direction = DMA_MEM_TO_DEV;
----> ret = dmaengine_slave_config(dma->chan_tx, &dma_sconfig);
if (ret < 0) {
dev_err(dev, "can't configure tx channel (%d)\n", ret);
goto fail_tx;
}
dma->chan_rx = dma_request_chan(dev, "rx");
if (IS_ERR(dma->chan_rx)) {
----> ret = PTR_ERR(dma->chan_rx);
if (ret != -ENODEV && ret != -EPROBE_DEFER)
dev_err(dev, "can't request DMA rx channel (%d)\n", ret);
goto fail_tx;
}
dma_sconfig.src_addr = phy_addr +
(IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
dma_sconfig.src_maxburst = 1;
dma_sconfig.direction = DMA_DEV_TO_MEM;
ret = dmaengine_slave_config(dma->chan_rx, &dma_sconfig);
if (ret < 0) {
dev_err(dev, "can't configure rx channel (%d)\n", ret);
goto fail_rx;
}
i2c_imx->dma = dma;
init_completion(&dma->cmd_complete);
dev_info(dev, "using %s (tx) and %s (rx) for DMA transfers\n",
dma_chan_name(dma->chan_tx), dma_chan_name(dma->chan_rx));
return 0;
fail_rx:
dma_release_channel(dma->chan_rx);
fail_tx:
dma_release_channel(dma->chan_tx);
fail_al:
devm_kfree(dev, dma);
return ret;
}
>
> --
> Pengutronix e.K. |
> |
> Steuerwalder Str. 21 |
> http://www.pen/
> gutronix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7Cd77fb08d389a
> 46773df008dd20ca5fdc%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
> 7C638702775737161676%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiO
> nRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%
> 3D%3D%7C0%7C%7C%7C&sdata=B%2BR4YbexNiAlDpf5xTf%2BWp2GbmwtfG4
> yKaW8FBvaTRI%3D&reserved=0 |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0
> |
> Amtsgericht Hildesheim, HRA 2686 | Fax:
> +49-5121-206917-5555 |
Powered by blists - more mailing lists