-----Original Message-----
From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Sent: 2019年12月11日 18:44
To: Peng Ma <redacted>
Cc: festevam@gmail.com; s.hauer@pengutronix.de;
linux-kernel@vger.kernel.org; linux@rempel-privat.de; dl-linux-imx
[off-list ref]; kernel@pengutronix.de; shawnguo@kernel.org;
linux-arm-kernel@lists.infradead.org; linux-i2c@vger.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:

quoted

Hi Russell,

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

Best Regards,
Peng

quoted

-----Original Message-----
From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Sent: 2019年11月28日 18:06
To: Peng Ma <redacted>
Cc: linux@rempel-privat.de; kernel@pengutronix.de;
shawnguo@kernel.org; s.hauer@pengutronix.de;
linux-kernel@vger.kernel.org; dl-linux-imx [off-list ref];
festevam@gmail.com; linux-arm-kernel@lists.infradead.org;
linux-i2c@vger.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:

quoted

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.

quoted

Signed-off-by: Peng Ma <redacted>
---
 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)

quoted

+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,

quoted

      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,

quoted

      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.

quoted

 }

 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.

quoted

quoted

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.

quoted

quoted

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.

quoted

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.

quoted

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&data=02%7C01%7Cpeng.ma
%40nxp.com%7C409ea9ad019e4cd5a62408d77e270577%7C686ea1d3bc2b4c
6fa92cd99c5c301635%7C0%7C0%7C637116578381480430&sdata=ohI%
2FQDgIlVfr%2FPJ3%2BLs1vIxbpwcxRpccKWdBI517RuU%3D&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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel