linux-kernel - RE: [EXT] Re: [PATCH] i2c: imx: Defer probing if EDMA not available

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]
Date:   Wed, 11 Dec 2019 10:25:26 +0000
From:   Peng Ma <peng.ma@....com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
CC:     "linux@...pel-privat.de" <linux@...pel-privat.de>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>,
        "festevam@...il.com" <festevam@...il.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>
Subject: RE: [EXT] Re: [PATCH] i2c: imx: Defer probing if EDMA not available

Hi Russell,

I am sorry to reply late, thanks for your patient reminding,
Please see my comments inline.

Best Regards,
Peng
>-----Original Message-----
>From: Russell King - ARM Linux admin <linux@...linux.org.uk>
>Sent: 2019年11月28日 18:06
>To: Peng Ma <peng.ma@....com>
>Cc: linux@...pel-privat.de; kernel@...gutronix.de; shawnguo@...nel.org;
>s.hauer@...gutronix.de; linux-kernel@...r.kernel.org; dl-linux-imx
><linux-imx@....com>; festevam@...il.com;
>linux-arm-kernel@...ts.infradead.org; linux-i2c@...r.kernel.org
>Subject: [EXT] Re: [PATCH] i2c: imx: Defer probing if EDMA not available
>
>Caution: EXT Email
>
>On Wed, Nov 27, 2019 at 07:12:09AM +0000, Peng Ma wrote:
>> EDMA may be not available or defered due to dependencies on other
>> modules, If these scenarios is encountered, we should defer probing.
>
>This has been tried before in this form, and it causes regressions.
>
>> Signed-off-by: Peng Ma <peng.ma@....com>
>> ---
>>  drivers/i2c/busses/i2c-imx.c | 16 +++++++++++-----
>>  1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-imx.c
>> b/drivers/i2c/busses/i2c-imx.c index 40111a3..c2b0693 100644
>> --- a/drivers/i2c/busses/i2c-imx.c
>> +++ b/drivers/i2c/busses/i2c-imx.c
>> @@ -369,8 +369,8 @@ static void i2c_imx_reset_regs(struct
>> imx_i2c_struct *i2c_imx)  }
>>
>>  /* Functions for DMA support */
>> -static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
>> -                                             dma_addr_t
>phy_addr)
>> +static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
>> +                            dma_addr_t phy_addr)
>>  {
>>       struct imx_i2c_dma *dma;
>>       struct dma_slave_config dma_sconfig; @@ -379,7 +379,7 @@ static
>> void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
>>
>>       dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL);
>>       if (!dma)
>> -             return;
>> +             return -ENOMEM;
>>
>>       dma->chan_tx = dma_request_chan(dev, "tx");
>>       if (IS_ERR(dma->chan_tx)) {
>> @@ -424,7 +424,7 @@ static void i2c_imx_dma_request(struct
>imx_i2c_struct *i2c_imx,
>>       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;
>> +     return 0;
>>
>>  fail_rx:
>>       dma_release_channel(dma->chan_rx);
>> @@ -432,6 +432,8 @@ static void i2c_imx_dma_request(struct
>imx_i2c_struct *i2c_imx,
>>       dma_release_channel(dma->chan_tx);
>>  fail_al:
>>       devm_kfree(dev, dma);
>> +
>> +     return ret;
>
>Some platforms don't have EDMA.  Doesn't this force everyone who wants
>I2C to have DMA?  The last attempt at this had:
>
>        /* return successfully if there is no dma support */
>        return ret == -ENODEV ? 0 : ret;
>
>here because of exactly this.
>
>>  }
>>
>>  static void i2c_imx_dma_callback(void *arg) @@ -1605,10 +1607,14 @@
>> static int i2c_imx_probe(struct platform_device *pdev)
>>       dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n");
>>
>>       /* Init DMA config if supported */
>> -     i2c_imx_dma_request(i2c_imx, phy_addr);
>> +     ret = i2c_imx_dma_request(i2c_imx, phy_addr);
>> +     if (ret == -EPROBE_DEFER)
>> +             goto i2c_adapter_remove;
>
>This happens _after_ the adapter has been published to the rest of the kernel.
>Claiming resources after publication is racy - the adapter may be in use by a
>request at this point.  Secondly, there's been problems with this causing
>regressions when EDMA is built as a module and i2c-imx is built-in.
>
>See e8c220fac415 ("Revert "i2c: imx: improve the error handling in
>i2c_imx_dma_request()"") when exactly what you're proposing was tried and
>ended up having to be reverted.
>
>AFAIK nothing has changed since, so merely reinstating the known to be broken
>code, thereby reintroducing the same (and more) problems, isn't going to be
>acceptable.
>
>Sorry, but this gets a big NAK from me.
>
[Peng Ma] I saw the revert commit e8c220fac415 and understand your concerns.
I scan the i2c-imx.c driver, All platforms that use i2c driver and support dma use an eDMA engine,
So I change the code(compare with last patch) as follows, please review and give me your precious comments.
Thanks very much.

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 12f7934fddb4..6cafee52dd67 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -1605,8 +1605,11 @@ static int i2c_imx_probe(struct platform_device *pdev)
 
        /* Init DMA config if supported */
        ret = i2c_imx_dma_request(i2c_imx, phy_addr);
-       if (ret == -EPROBE_DEFER)
+       if (ret == -EPROBE_DEFER) {
+#if    IS_BUILTIN(CONFIG_FSL_EDMA)
                goto i2c_adapter_remove;
+#endif
+       }

>>
>>       return 0;   /* Return OK */
>>
>> +i2c_adapter_remove:
>> +     i2c_del_adapter(&i2c_imx->adapter);
>>  clk_notifier_unregister:
>>       clk_notifier_unregister(i2c_imx->clk, &i2c_imx->clk_change_nb);
>>  rpm_disable:
>> --
>> 2.9.5
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@...ts.infradead.org
>> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists
>> .infradead.org%2Fmailman%2Flistinfo%2Flinux-arm-kernel&amp;data=02%7
>C0
>>
>1%7Cpeng.ma%40nxp.com%7C6dbcf73ceb10495457fa08d773ea9ee1%7C686
>ea1d3bc2
>>
>b4c6fa92cd99c5c301635%7C0%7C1%7C637105323843174631&amp;sdata=d
>v1UINRME
>> Cx6w2xG%2FyliNWNvIbTbacHpqAt8%2B6W5qFk%3D&amp;reserved=0
>>
>
>--
>RMK's Patch system:
>https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.ar
>mlinux.org.uk%2Fdeveloper%2Fpatches%2F&amp;data=02%7C01%7Cpeng.ma
>%40nxp.com%7C6dbcf73ceb10495457fa08d773ea9ee1%7C686ea1d3bc2b4c6
>fa92cd99c5c301635%7C0%7C1%7C637105323843184629&amp;sdata=9FZCA
>JJxE99wP5ZMoG6ib%2FeYoXdksgq2uSzBrBtNUnU%3D&amp;reserved=0
>FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps
>up According to speedtest.net: 11.9Mbps down 500kbps up