Date:   Wed, 11 Dec 2019 11:22:00 +0000
From:   Peng Ma <peng.ma@....com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
CC:     "festevam@...il.com" <festevam@...il.com>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux@...pel-privat.de" <linux@...pel-privat.de>,
        dl-linux-imx <linux-imx@....com>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "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



>-----Original Message-----
>From: Russell King - ARM Linux admin <linux@...linux.org.uk>
>Sent: 2019年12月11日 18:44
>To: Peng Ma <peng.ma@....com>
>Cc: festevam@...il.com; s.hauer@...gutronix.de;
>linux-kernel@...r.kernel.org; linux@...pel-privat.de; dl-linux-imx
><linux-imx@....com>; kernel@...gutronix.de; shawnguo@...nel.org;
>linux-arm-kernel@...ts.infradead.org; linux-i2c@...r.kernel.org
>Subject: Re: [EXT] Re: [PATCH] i2c: imx: Defer probing if EDMA not available
>
>Caution: EXT Email
>
>On Wed, Dec 11, 2019 at 10:25:26AM +0000, Peng Ma wrote:
>> 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
>> +       }
>
>You haven't understood _why_ the problem occurs, you're just attempting to
>patch around it. You're hacking the code, rather than engineering the code.
>
>The infinite deferred probe occurs because:
>
>- i2c-imx is attempted to be probed.
>- i2c-imx sets up the hardware, and then calls
>  i2c_add_numbered_adapter()
>- i2c_add_numbered_adapter() publishes the bus to the world, and then
>  searches DT for any children to create - and it finds some and
>  creates them.
>- the children devices are matched to their drivers, which bind.  This
>  triggers a deferred probe to be scheduled.
>- back in the i2c-imx driver, we get to i2c_imx_dma_request(), which
>  fails, and you return -EPROBE_DEFER.
>- the i2c-imx driver probe actions are unwound, and probe exits.
>- the driver core processes the deferred probe request, finds the
>  i2c-imx device(s) on the deferred probe list, and attempts to
>  probe them.  Goto the top of this list.
>
[Peng Ma] Thanks for your quick reply, No, I don't think so, when first,second,third...... time probe failed, the i2c_del_adapter will be called(it will remove the i2c children device). I think if We build-in EDMA, after EDMA probe successful, the deffer probe of i2c will probe with no return -EPROBE_DEFER.
So you say " Goto the top of this list " just i2c drive probe failed with i2c_imx_dma_request() return -EPROBE_DEFER,
If the EDMA build-in and probe successful this case not happened. Now I am worried about EDMA failed to probe, your case is correct.
>If, for whatever reason, i2c_imx_dma_request() ever returns -EPROBE_DEFER,
>the above loop WILL happen.
>
>The FUNDAMENTAL rule of kernel programming is that you do NOT publish
>before you have completed setup.  i2c-imx violates that rule as the probe
>function is ordered at present.
>
[Peng Ma] Yes, I agree, but kernel provide the deffer probe and for the platform devices we don't decide who probe first.
>i2c-imx has been written for i2c_imx_dma_request() to be safe to call after the
>device has been published, but with the current probe function order, it is
>unsafe to propagate the EPROBE_DEFER return value for the reason above.
>For the reason the original attempt got reverted.
>
>So, if you want to do this (and yes, I'd also encourage it to be conditional on
>EDMA being built-in, as I2C is commonly used as a way to get at RTCs, which
>are read before kernel modules can be loaded) then you MUST move
>i2c_imx_dma_request() before
>i2c_add_numbered_adapter() to avoid the infinite loop.
>
[Peng Ma] To do this, the i2c devices not probe and i2c adapter not register before edma probe.

Best Regards,
Peng
>--
>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%7C409ea9ad019e4cd5a62408d77e270577%7C686ea1d3bc2b4c
>6fa92cd99c5c301635%7C0%7C0%7C637116578381480430&amp;sdata=ohI%
>2FQDgIlVfr%2FPJ3%2BLs1vIxbpwcxRpccKWdBI517RuU%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

Powered by blists - more mailing lists