i2c: imx: Defer probing if EDMA not available

Commit Message

Peng Ma Nov. 27, 2019, 7:12 a.m. UTC

EDMA may be not available or defered due to dependencies on
other modules, If these scenarios is encountered, we should
defer probing.

Signed-off-by: Peng Ma <[e-mail]>
---
 drivers/i2c/busses/i2c-imx.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Uwe Kleine-König Nov. 27, 2019, 7:27 a.m. UTC | #1

Hello, 

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.

I'd write:

	i2c: imx: Defer probing if requesting DMA yields EPROBE_DEFER

	DMA might not be available yet when the i2c device probes.
	Properly handle EPROBE_DEFER on dma channel allocation by
	passing on this error.

It would be nice to point out where/how this failed for you.

Other than that this looks reasonable.

Best regards
Uwe

Peng Ma Nov. 28, 2019, 2:45 a.m. UTC | #2

>-----Original Message-----
>From: Uwe Kleine-König <[e-mail]>
>Sent: 2019年11月27日 15:27
>To: Peng Ma <[e-mail]>
>Cc: [e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; dl-linux-imx
><[e-mail]>; [e-mail];
>[e-mail]; [e-mail]
>Subject: [EXT] Re: [PATCH] i2c: imx: Defer probing if EDMA not available
>
>Caution: EXT Email
>
>Hello,
>
>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.
>
>I'd write:
>
>        i2c: imx: Defer probing if requesting DMA yields EPROBE_DEFER
>
>        DMA might not be available yet when the i2c device probes.
>        Properly handle EPROBE_DEFER on dma channel allocation by
>        passing on this error.
>
>It would be nice to point out where/how this failed for you.
>
>Other than that this looks reasonable.
>
Hi, Uwe,

Thanks very much for your comments, That looks good.

Best Regards,
Peng
>Best regards
>Uwe
>
>--
>Pengutronix e.K.                           | Uwe Kleine-König
>|
>Industrial Linux Solutions                 |
>https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.p
>engutronix.de%2F&amp;data=02%7C01%7Cpeng.ma%40nxp.com%7C426ec4f
>5f1f14751022f08d7730b4043%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
>%7C1%7C637104364470568722&amp;sdata=JjD2ovy4oxZseUEUwNERASBJCk
>0oMLSplb8qRTkhaTE%3D&amp;reserved=0 |

Russell King (Oracle) Nov. 28, 2019, 10:06 a.m. UTC | #3

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 <[e-mail]>
> ---
>  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.

>  
>  	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
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Peng Ma Dec. 11, 2019, 10:25 a.m. UTC | #4

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 <[e-mail]>
>Sent: 2019年11月28日 18:06
>To: Peng Ma <[e-mail]>
>Cc: [e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; dl-linux-imx
><[e-mail]>; [e-mail];
>[e-mail]; [e-mail]
>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 <[e-mail]>
>> ---
>>  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
>> [e-mail]
>> 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

Russell King (Oracle) Dec. 11, 2019, 10:43 a.m. UTC | #5

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 <[e-mail]>
> >Sent: 2019年11月28日 18:06
> >To: Peng Ma <[e-mail]>
> >Cc: [e-mail]; [e-mail]; [e-mail];
> >[e-mail]; [e-mail]; dl-linux-imx
> ><[e-mail]>; [e-mail];
> >[e-mail]; [e-mail]
> >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 <[e-mail]>
> >> ---
> >>  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.

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.

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 Dec. 11, 2019, 11:22 a.m. UTC | #6

>-----Original Message-----
>From: Russell King - ARM Linux admin <[e-mail]>
>Sent: 2019年12月11日 18:44
>To: Peng Ma <[e-mail]>
>Cc: [e-mail]; [e-mail];
>[e-mail]; [e-mail]; dl-linux-imx
><[e-mail]>; [e-mail]; [e-mail];
>[e-mail]; [e-mail]
>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 <[e-mail]>
>> >Sent: 2019年11月28日 18:06
>> >To: Peng Ma <[e-mail]>
>> >Cc: [e-mail]; [e-mail];
>> >[e-mail]; [e-mail];
>> >[e-mail]; dl-linux-imx <[e-mail]>;
>> >[e-mail]; [e-mail];
>> >[e-mail]
>> >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 <[e-mail]>
>> >> ---
>> >>  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

Russell King (Oracle) Dec. 11, 2019, 11:42 a.m. UTC | #7

On Wed, Dec 11, 2019 at 11:22:00AM +0000, Peng Ma wrote:
> 
> 
> >-----Original Message-----
> >From: Russell King - ARM Linux admin <[e-mail]>
> >Sent: 2019年12月11日 18:44
> >To: Peng Ma <[e-mail]>
> >Cc: [e-mail]; [e-mail];
> >[e-mail]; [e-mail]; dl-linux-imx
> ><[e-mail]>; [e-mail]; [e-mail];
> >[e-mail]; [e-mail]
> >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 <[e-mail]>
> >> >Sent: 2019年11月28日 18:06
> >> >To: Peng Ma <[e-mail]>
> >> >Cc: [e-mail]; [e-mail];
> >> >[e-mail]; [e-mail];
> >> >[e-mail]; dl-linux-imx <[e-mail]>;
> >> >[e-mail]; [e-mail];
> >> >[e-mail]
> >> >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 <[e-mail]>
> >> >> ---
> >> >>  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.

Yes, i2c_del_adapter will be called, but that is neither here nor there.
The deferred probe is triggered by _any_ driver binding.  The facts are:

i2c_add_numbered_adapter() creates devices.
These new devices get bound to drivers.
As soon as any one of those devices binds to a driver, deferred probing
is triggered.
When i2c_imx_probe() returns -EPROBE_DEFER, it will be added to the list
of devices to be re-probed by the deferred probing.

> 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.

You are assuming that EDMA has successfully probed. What if EDMA hasn't
been probed yet, because it has been deferred for some other reason (e.g.
a clock)?

The fact is, the way i2c-imx is structured at present, it is unsafe to
propagate the EPROBE_DEFER error code from i2c_imx_dma_request() under
ANY CIRCUMSTANCES.

> >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.

So, because the kernel provides a facility, you think it's fine to
create infinite loops using it?

> >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.

Which is the correct behaviour, rather than having the kernel cycle
through creating i2c devices, probing i2c drivers, tearing down the
i2c devices and repeating endlessly.

Until you see this, sorry, no, you can't propagate the return value
from i2c_imx_dma_request().  We've tried it, it's caused regressions,
and a problem has been identified that you don't seem to be willing
to recognise _as_ a serious problem with the approach you're trying
to re-implement.

Peng Ma Dec. 12, 2019, 3:09 a.m. UTC | #8

Hello Russell,

Thanks very much for your strict guidance and comments.
I realized it is hard to us that we want to i2c used edma when edma
probe after i2c probe. I look forward to discussing with you as below, if you like.
Thanks.

You say I could do this:
"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."

Even if I do this, It's hard to avoid the infinite loop of i2c probe caused by EDMA(build-in) initialization failure.
I saw the function of_dma_request_slave_channel alloc chan as fallows:

ofdma = of_dma_find_controller(&dma_spec);

        if (ofdma) {
            chan = ofdma->of_dma_xlate(&dma_spec, ofdma);//dma probe successful when devices alloc dma slave channel
        } else {
            ret_no_channel = -EPROBE_DEFER; //dma not probe or probe failed when devices alloc dma slave channel
            chan = NULL;
        }   

Due to this case,we should make sure:
1. EDMA build-in
2. EDMA can probe successful

The first can realize, but I don't know how to check whether EDMA has been probed failed in i2c,
If we could check it, i2c will skip that loop.

Best Regards,
Peng
>-----Original Message-----
>From: Russell King - ARM Linux admin <[e-mail]>
>Sent: 2019年12月11日 19:43
>To: Peng Ma <[e-mail]>
>Cc: [e-mail]; [e-mail];
>[e-mail]; [e-mail]; dl-linux-imx
><[e-mail]>; [e-mail]; [e-mail];
>[e-mail]; [e-mail]
>Subject: Re: [EXT] Re: [PATCH] i2c: imx: Defer probing if EDMA not available
>
>Caution: EXT Email
>
>On Wed, Dec 11, 2019 at 11:22:00AM +0000, Peng Ma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Russell King - ARM Linux admin <[e-mail]>
>> >Sent: 2019年12月11日 18:44
>> >To: Peng Ma <[e-mail]>
>> >Cc: [e-mail]; [e-mail];
>> >[e-mail]; [e-mail]; dl-linux-imx
>> ><[e-mail]>; [e-mail]; [e-mail];
>> >[e-mail]; [e-mail]
>> >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 <[e-mail]>
>> >> >Sent: 2019年11月28日 18:06
>> >> >To: Peng Ma <[e-mail]>
>> >> >Cc: [e-mail]; [e-mail];
>> >> >[e-mail]; [e-mail];
>> >> >[e-mail]; dl-linux-imx <[e-mail]>;
>> >> >[e-mail]; [e-mail];
>> >> >[e-mail]
>> >> >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 <[e-mail]>
>> >> >> ---
>> >> >>  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.
>
>Yes, i2c_del_adapter will be called, but that is neither here nor there.
>The deferred probe is triggered by _any_ driver binding.  The facts are:
>
>i2c_add_numbered_adapter() creates devices.
>These new devices get bound to drivers.
>As soon as any one of those devices binds to a driver, deferred probing is
>triggered.
>When i2c_imx_probe() returns -EPROBE_DEFER, it will be added to the list of
>devices to be re-probed by the deferred probing.
>
>> 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.
>
>You are assuming that EDMA has successfully probed. What if EDMA hasn't
>been probed yet, because it has been deferred for some other reason (e.g.
>a clock)?
>
>The fact is, the way i2c-imx is structured at present, it is unsafe to propagate
>the EPROBE_DEFER error code from i2c_imx_dma_request() under ANY
>CIRCUMSTANCES.
>
>> >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.
>
>So, because the kernel provides a facility, you think it's fine to create infinite
>loops using it?
>
>> >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.
>
>Which is the correct behaviour, rather than having the kernel cycle through
>creating i2c devices, probing i2c drivers, tearing down the i2c devices and
>repeating endlessly.
>
>Until you see this, sorry, no, you can't propagate the return value from
>i2c_imx_dma_request().  We've tried it, it's caused regressions, and a
>problem has been identified that you don't seem to be willing to recognise _as_
>a serious problem with the approach you're trying to re-implement.
>
>--
>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%7C142e002eba8940b2250b08d77e2f3999%7C686ea1d3bc2b4c
>6fa92cd99c5c301635%7C0%7C0%7C637116613612463295&amp;sdata=AdIo
>q6rNyPDcKAjq%2FikDhE8vp3OpyGOUV108TO6r4jo%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

Russell King (Oracle) Dec. 12, 2019, 10:58 a.m. UTC | #9

On Thu, Dec 12, 2019 at 03:09:32AM +0000, Peng Ma wrote:
> Hello Russell,
> 
> Thanks very much for your strict guidance and comments.
> I realized it is hard to us that we want to i2c used edma when edma
> probe after i2c probe.

I have no problem with that aim.  I'm just very concerned by the
proposed implementation, especially when it has already been proven
to cause regressions in the kernel. I seem to remember that the
infinite loop caused other issues, such as the system being unable
to complete booting.

> I look forward to discussing with you as below, if you like.
> Thanks.
> 
> You say I could do this:
> "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."
> 
> Even if I do this, It's hard to avoid the infinite loop of i2c probe caused by EDMA(build-in) initialization failure.

It isn't clear what you mean here.

If EDMA fails to probe (because fsl_edma_probe() returns an error other
than EPROBE_DEFER) then of_dma_find_controller() will return NULL. That
will be propagated down through i2c_imx_dma_request(). This is no
different from the case where EDMA is built as a module. It is also no
different from the case where EDMA hasn't yet been probed.

If i2c_imx_dma_request() is placed after i2c_add_numbered_adapter(),
and EPROBE_DEFER is propagated out of i2c_imx_probe(), then _yes_, it
will cause an infinite loop, because you are replicating the exact
conditions that caused the attempt to propagate i2c_imx_dma_request()'s
return value to be reverted last time - which brought the kernel to a
grinding halt.

If i2c_imx_dma_request() is placed before i2c_add_numbered_adapter(),
then there is no infinite deferred probing loop - yes, i2c_imx_probe()
will be called as a result of other drivers successfully probing, and
each time it will return EPROBE_DEFER, but the _key_ point is that
the action of i2c_imx_probe() will not _self trigger_ the deferred
probing _and_ place itself onto the deferred probe list.

Please, rather than continuing to send emails arguing over this point,
investigate the stated issue with some practical tests:

1. Make i2c_imx_probe() propagate i2c_imx_dma_request()'s return value,
   as it did in the original patch.
2. Build i2c-imx into the kernel.
3. Build edma as a module.
4. Build and test boot the kernel and check what happens.
5. Move i2c_imx_dma_request() before i2c_add_numbered_adapter()
6. Build and test boot the resulting kernel and note any differences.

Peng Ma Dec. 13, 2019, 10:33 a.m. UTC | #10

>-----Original Message-----
>From: Russell King - ARM Linux admin <[e-mail]>
>Sent: 2019年12月12日 18:59
>To: Peng Ma <[e-mail]>
>Cc: [e-mail]; [e-mail];
>[e-mail]; [e-mail]; Abel Vesa
><[e-mail]>; Aisheng Dong <[e-mail]>; Anson Huang
><[e-mail]>; Bogdan Florin Vlad <[e-mail]>;
>BOUGH CHEN <[e-mail]>; Clark Wang
><[e-mail]>; Daniel Baluta <[e-mail]>; Fancy
>Fang <[e-mail]>; Han Xu <[e-mail]>; Horia Geanta
><[e-mail]>; Iuliana Prodan <[e-mail]>; Jacky
>Bai <[e-mail]>; Joakim Zhang <[e-mail]>; Jun Li
><[e-mail]>; Leo Zhang <[e-mail]>; Leonard Crestez
><[e-mail]>; Mircea Pop <[e-mail]>; Mirela
>Rabulea <[e-mail]>; Peng Fan <[e-mail]>; Peter
>Chen <[e-mail]>; Ranjani Vaidyanathan
><[e-mail]>; Robert Chiras <[e-mail]>;
>Robin Gong <[e-mail]>; Shenwei Wang
><[e-mail]>; Viorel Suman <[e-mail]>; Ying Liu
><[e-mail]>; Zening Wang <[e-mail]>;
>[e-mail]; [e-mail];
>[e-mail]; [e-mail]
>Subject: Re: [EXT] Re: [PATCH] i2c: imx: Defer probing if EDMA not available
>
>Caution: EXT Email
>
>On Thu, Dec 12, 2019 at 03:09:32AM +0000, Peng Ma wrote:
>> Hello Russell,
>>
>> Thanks very much for your strict guidance and comments.
>> I realized it is hard to us that we want to i2c used edma when edma
>> probe after i2c probe.
>
>I have no problem with that aim.  I'm just very concerned by the proposed
>implementation, especially when it has already been proven to cause
>regressions in the kernel. I seem to remember that the infinite loop caused
>other issues, such as the system being unable to complete booting.
>
>> I look forward to discussing with you as below, if you like.
>> Thanks.
>>
>> You say I could do this:
>> "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."
>>
>> Even if I do this, It's hard to avoid the infinite loop of i2c probe caused by
>EDMA(build-in) initialization failure.
>
>It isn't clear what you mean here.
>
>If EDMA fails to probe (because fsl_edma_probe() returns an error other than
>EPROBE_DEFER) then of_dma_find_controller() will return NULL. That will be
>propagated down through i2c_imx_dma_request(). This is no different from the
>case where EDMA is built as a module. It is also no different from the case
>where EDMA hasn't yet been probed.
>
Hello Russell,

The result of my test is not like that, It is still with probe loop, the test config as follows:
1.EDMA build-in
2.return -EINVAL top of fsl_edma_probe when edma probe
3.i2c probe with original patch, I put the i2c_imx_dma_request in front of i2c_add_numbered_adapter or used original patch.

I send you the function of_dma_request_slave_channel could explain it last mail,
"Return -EPROBE_DEFER" depends on:
1. edma not probe or probe failed
2. There is edma node in DTS and I2C with edma property

>If i2c_imx_dma_request() is placed after i2c_add_numbered_adapter(), and
>EPROBE_DEFER is propagated out of i2c_imx_probe(), then _yes_, it will cause
>an infinite loop, because you are replicating the exact conditions that caused
>the attempt to propagate i2c_imx_dma_request()'s return value to be reverted
>last time - which brought the kernel to a grinding halt.
>
>If i2c_imx_dma_request() is placed before i2c_add_numbered_adapter(), then
>there is no infinite deferred probing loop - yes, i2c_imx_probe() will be called as
>a result of other drivers successfully probing, and each time it will return
>EPROBE_DEFER, but the _key_ point is that the action of i2c_imx_probe() will
>not _self trigger_ the deferred probing _and_ place itself onto the deferred
>probe list.
>
>Please, rather than continuing to send emails arguing over this point,
>investigate the stated issue with some practical tests:
>
>1. Make i2c_imx_probe() propagate i2c_imx_dma_request()'s return value,
>   as it did in the original patch.
>2. Build i2c-imx into the kernel.
>3. Build edma as a module.
>4. Build and test boot the kernel and check what happens.
>5. Move i2c_imx_dma_request() before i2c_add_numbered_adapter() 6. Build
>and test boot the resulting kernel and note any differences.
>
[Peng Ma] the i2c probe loop still exist but not infinite loop with below cases:
1: 
	Used original patch
	Build i2c-imx into the kernel
	Build edma into the kernel
2:
	Used original patch
	Build i2c-imx into the kernel
	Build edma into the kernel
	Move i2c_imx_dma_request() before i2c_add_numbered_adapter()
3:
	Used original patch
	Build i2c-imx into the kernel
	Build edma as a module
	Move i2c_imx_dma_request() before i2c_add_numbered_adapter()

I saw the commit e8c220fac415d9f4a994b0c2871b835feac1eb4e you said
1. i2c_imx_probe() is called and successfully registers an I2C
   adapter via i2c_add_numbered_adapter()

2. As a part of i2c_add_numbered_adapter() new I2C slave devices
   are added from DT which results in a call to
   driver_deferred_probe_trigger()

3. i2c_imx_probe() continues and calls i2c_imx_dma_request() which
   due to lack of proper DMA driver returns -EPROBE_DEFER

4. i2c_imx_probe() fails, removes I2C adapter and returns
   -EPROBE_DEFER, which places it into deferred probe list

5. Deferred probe work triggered in #2 above kicks in and calls
   i2c_imx_probe() again thus bringing us to step #1"
Can I understand 4, you mean just remove I2C adapter the i2c slave devices not be removed? Then the i2c slave devices will probe their drivers triggered the driver_deferred_probe_trigger().
If so, My test is not like this, when the i2c probe failed the i2c slave devices will be removed and I2C adapter will be removed too.

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%7Ca535712eabe343a51c2f08d77ef250e4%7C686ea1d3bc2b4c6
>fa92cd99c5c301635%7C0%7C0%7C637117451627341155&amp;sdata=%2Fyz
>xAI8%2FezVRwGTaT4vYa3CMkIMsaSYiaH8DjvJWUKA%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

Russell King (Oracle) Dec. 13, 2019, 10:50 a.m. UTC | #11

On Fri, Dec 13, 2019 at 10:33:51AM +0000, Peng Ma wrote:
> 
> 
> >-----Original Message-----
> >From: Russell King - ARM Linux admin <[e-mail]>
> >Sent: 2019年12月12日 18:59
> >To: Peng Ma <[e-mail]>
> >Cc: [e-mail]; [e-mail];
> >[e-mail]; [e-mail]; Abel Vesa
> ><[e-mail]>; Aisheng Dong <[e-mail]>; Anson Huang
> ><[e-mail]>; Bogdan Florin Vlad <[e-mail]>;
> >BOUGH CHEN <[e-mail]>; Clark Wang
> ><[e-mail]>; Daniel Baluta <[e-mail]>; Fancy
> >Fang <[e-mail]>; Han Xu <[e-mail]>; Horia Geanta
> ><[e-mail]>; Iuliana Prodan <[e-mail]>; Jacky
> >Bai <[e-mail]>; Joakim Zhang <[e-mail]>; Jun Li
> ><[e-mail]>; Leo Zhang <[e-mail]>; Leonard Crestez
> ><[e-mail]>; Mircea Pop <[e-mail]>; Mirela
> >Rabulea <[e-mail]>; Peng Fan <[e-mail]>; Peter
> >Chen <[e-mail]>; Ranjani Vaidyanathan
> ><[e-mail]>; Robert Chiras <[e-mail]>;
> >Robin Gong <[e-mail]>; Shenwei Wang
> ><[e-mail]>; Viorel Suman <[e-mail]>; Ying Liu
> ><[e-mail]>; Zening Wang <[e-mail]>;
> >[e-mail]; [e-mail];
> >[e-mail]; [e-mail]
> >Subject: Re: [EXT] Re: [PATCH] i2c: imx: Defer probing if EDMA not available
> >
> >Caution: EXT Email
> >
> >On Thu, Dec 12, 2019 at 03:09:32AM +0000, Peng Ma wrote:
> >> Hello Russell,
> >>
> >> Thanks very much for your strict guidance and comments.
> >> I realized it is hard to us that we want to i2c used edma when edma
> >> probe after i2c probe.
> >
> >I have no problem with that aim.  I'm just very concerned by the proposed
> >implementation, especially when it has already been proven to cause
> >regressions in the kernel. I seem to remember that the infinite loop caused
> >other issues, such as the system being unable to complete booting.
> >
> >> I look forward to discussing with you as below, if you like.
> >> Thanks.
> >>
> >> You say I could do this:
> >> "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."
> >>
> >> Even if I do this, It's hard to avoid the infinite loop of i2c probe caused by
> >EDMA(build-in) initialization failure.
> >
> >It isn't clear what you mean here.
> >
> >If EDMA fails to probe (because fsl_edma_probe() returns an error other than
> >EPROBE_DEFER) then of_dma_find_controller() will return NULL. That will be
> >propagated down through i2c_imx_dma_request(). This is no different from the
> >case where EDMA is built as a module. It is also no different from the case
> >where EDMA hasn't yet been probed.
> >
> Hello Russell,
> 
> The result of my test is not like that, It is still with probe loop, the test config as follows:

So you haven't tested the scenario that causes the problem.  How
convenient for you.

> 1.EDMA build-in
> 2.return -EINVAL top of fsl_edma_probe when edma probe
> 3.i2c probe with original patch, I put the i2c_imx_dma_request in front of i2c_add_numbered_adapter or used original patch.
> 
> I send you the function of_dma_request_slave_channel could explain it last mail,
> "Return -EPROBE_DEFER" depends on:
> 1. edma not probe or probe failed
> 2. There is edma node in DTS and I2C with edma property

Correct.

I'm sorry, but my patience is wearing very thin. I've explained the
problem in detail, I've explained how you can reproduce it, but it
seems I'm not being listened to. So, I don't have anything further to
add to this discussion that hasn't already been said.

Consider any patch that adds *any* path that can return -EPROBE_DEFER
after a successful call to i2c_add_numbered_adapter() or its similar
functions to be NAK'd by myself on account of this infinite probe loop
that has been proven in previous kernels to occur.

Thanks.

Patch

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;
 }
 
 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;
 
 	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: