* [PATCH] i2c: imx: Defer probing if EDMA not available @ 2019-11-27 7:12 Peng Ma 2019-11-27 7:27 ` Uwe Kleine-König 2019-11-28 10:06 ` Russell King - ARM Linux admin 0 siblings, 2 replies; 12+ messages in thread From: Peng Ma @ 2019-11-27 7:12 UTC (permalink / raw) To: linux, kernel, shawnguo, s.hauer Cc: festevam, dl-linux-imx, linux-i2c, linux-arm-kernel, linux-kernel, Peng Ma 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 <peng.ma@nxp.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; } 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: -- 2.9.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] i2c: imx: Defer probing if EDMA not available 2019-11-27 7:12 [PATCH] i2c: imx: Defer probing if EDMA not available Peng Ma @ 2019-11-27 7:27 ` Uwe Kleine-König 2019-11-28 2:45 ` [EXT] " Peng Ma 2019-11-28 10:06 ` Russell King - ARM Linux admin 1 sibling, 1 reply; 12+ messages in thread From: Uwe Kleine-König @ 2019-11-27 7:27 UTC (permalink / raw) To: Peng Ma Cc: linux, kernel, shawnguo, s.hauer, linux-kernel, dl-linux-imx, festevam, linux-arm-kernel, linux-i2c 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 -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [EXT] Re: [PATCH] i2c: imx: Defer probing if EDMA not available 2019-11-27 7:27 ` Uwe Kleine-König @ 2019-11-28 2:45 ` Peng Ma 0 siblings, 0 replies; 12+ messages in thread From: Peng Ma @ 2019-11-28 2:45 UTC (permalink / raw) To: Uwe Kleine-König Cc: linux, kernel, shawnguo, s.hauer, linux-kernel, dl-linux-imx, festevam, linux-arm-kernel, linux-i2c >-----Original Message----- >From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >Sent: 2019年11月27日 15:27 >To: Peng Ma <peng.ma@nxp.com> >Cc: linux@rempel-privat.de; kernel@pengutronix.de; shawnguo@kernel.org; >s.hauer@pengutronix.de; linux-kernel@vger.kernel.org; dl-linux-imx ><linux-imx@nxp.com>; 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 > >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&data=02%7C01%7Cpeng.ma%40nxp.com%7C426ec4f >5f1f14751022f08d7730b4043%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0 >%7C1%7C637104364470568722&sdata=JjD2ovy4oxZseUEUwNERASBJCk >0oMLSplb8qRTkhaTE%3D&reserved=0 | ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] i2c: imx: Defer probing if EDMA not available 2019-11-27 7:12 [PATCH] i2c: imx: Defer probing if EDMA not available Peng Ma 2019-11-27 7:27 ` Uwe Kleine-König @ 2019-11-28 10:06 ` Russell King - ARM Linux admin 2019-12-11 10:25 ` [EXT] " Peng Ma 1 sibling, 1 reply; 12+ messages in thread From: Russell King - ARM Linux admin @ 2019-11-28 10:06 UTC (permalink / raw) To: Peng Ma Cc: linux, kernel, shawnguo, s.hauer, linux-kernel, dl-linux-imx, festevam, linux-arm-kernel, linux-i2c 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@nxp.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. > > 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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [EXT] Re: [PATCH] i2c: imx: Defer probing if EDMA not available 2019-11-28 10:06 ` Russell King - ARM Linux admin @ 2019-12-11 10:25 ` Peng Ma 2019-12-11 10:43 ` Russell King - ARM Linux admin 0 siblings, 1 reply; 12+ messages in thread From: Peng Ma @ 2019-12-11 10:25 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: linux, kernel, shawnguo, s.hauer, linux-kernel, dl-linux-imx, festevam, linux-arm-kernel, linux-i2c 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@armlinux.org.uk> >Sent: 2019年11月28日 18:06 >To: Peng Ma <peng.ma@nxp.com> >Cc: linux@rempel-privat.de; kernel@pengutronix.de; shawnguo@kernel.org; >s.hauer@pengutronix.de; linux-kernel@vger.kernel.org; dl-linux-imx ><linux-imx@nxp.com>; 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: >> 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@nxp.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@lists.infradead.org >> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists >> .infradead.org%2Fmailman%2Flistinfo%2Flinux-arm-kernel&data=02%7 >C0 >> >1%7Cpeng.ma%40nxp.com%7C6dbcf73ceb10495457fa08d773ea9ee1%7C686 >ea1d3bc2 >> >b4c6fa92cd99c5c301635%7C0%7C1%7C637105323843174631&sdata=d >v1UINRME >> Cx6w2xG%2FyliNWNvIbTbacHpqAt8%2B6W5qFk%3D&reserved=0 >> > >-- >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%7C6dbcf73ceb10495457fa08d773ea9ee1%7C686ea1d3bc2b4c6 >fa92cd99c5c301635%7C0%7C1%7C637105323843184629&sdata=9FZCA >JJxE99wP5ZMoG6ib%2FeYoXdksgq2uSzBrBtNUnU%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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [EXT] Re: [PATCH] i2c: imx: Defer probing if EDMA not available 2019-12-11 10:25 ` [EXT] " Peng Ma @ 2019-12-11 10:43 ` Russell King - ARM Linux admin 2019-12-11 11:22 ` Peng Ma 0 siblings, 1 reply; 12+ messages in thread From: Russell King - ARM Linux admin @ 2019-12-11 10:43 UTC (permalink / raw) To: Peng Ma Cc: festevam, s.hauer, linux-kernel, linux, dl-linux-imx, kernel, shawnguo, linux-arm-kernel, linux-i2c 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@armlinux.org.uk> > >Sent: 2019年11月28日 18:06 > >To: Peng Ma <peng.ma@nxp.com> > >Cc: linux@rempel-privat.de; kernel@pengutronix.de; shawnguo@kernel.org; > >s.hauer@pengutronix.de; linux-kernel@vger.kernel.org; dl-linux-imx > ><linux-imx@nxp.com>; 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: > >> 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@nxp.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. 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. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [EXT] Re: [PATCH] i2c: imx: Defer probing if EDMA not available 2019-12-11 10:43 ` Russell King - ARM Linux admin @ 2019-12-11 11:22 ` Peng Ma 2019-12-11 11:42 ` Russell King - ARM Linux admin 0 siblings, 1 reply; 12+ messages in thread From: Peng Ma @ 2019-12-11 11:22 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: festevam, s.hauer, linux-kernel, linux, dl-linux-imx, kernel, shawnguo, linux-arm-kernel, linux-i2c >-----Original Message----- >From: Russell King - ARM Linux admin <linux@armlinux.org.uk> >Sent: 2019年12月11日 18:44 >To: Peng Ma <peng.ma@nxp.com> >Cc: festevam@gmail.com; s.hauer@pengutronix.de; >linux-kernel@vger.kernel.org; linux@rempel-privat.de; dl-linux-imx ><linux-imx@nxp.com>; 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: >> 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@armlinux.org.uk> >> >Sent: 2019年11月28日 18:06 >> >To: Peng Ma <peng.ma@nxp.com> >> >Cc: linux@rempel-privat.de; kernel@pengutronix.de; >> >shawnguo@kernel.org; s.hauer@pengutronix.de; >> >linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; >> >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: >> >> 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@nxp.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&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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [EXT] Re: [PATCH] i2c: imx: Defer probing if EDMA not available 2019-12-11 11:22 ` Peng Ma @ 2019-12-11 11:42 ` Russell King - ARM Linux admin 2019-12-12 3:09 ` Peng Ma 0 siblings, 1 reply; 12+ messages in thread From: Russell King - ARM Linux admin @ 2019-12-11 11:42 UTC (permalink / raw) To: Peng Ma Cc: shawnguo, s.hauer, linux-kernel, linux, dl-linux-imx, kernel, festevam, linux-arm-kernel, linux-i2c On Wed, Dec 11, 2019 at 11:22:00AM +0000, Peng Ma wrote: > > > >-----Original Message----- > >From: Russell King - ARM Linux admin <linux@armlinux.org.uk> > >Sent: 2019年12月11日 18:44 > >To: Peng Ma <peng.ma@nxp.com> > >Cc: festevam@gmail.com; s.hauer@pengutronix.de; > >linux-kernel@vger.kernel.org; linux@rempel-privat.de; dl-linux-imx > ><linux-imx@nxp.com>; 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: > >> 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@armlinux.org.uk> > >> >Sent: 2019年11月28日 18:06 > >> >To: Peng Ma <peng.ma@nxp.com> > >> >Cc: linux@rempel-privat.de; kernel@pengutronix.de; > >> >shawnguo@kernel.org; s.hauer@pengutronix.de; > >> >linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; > >> >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: > >> >> 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@nxp.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. 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://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [EXT] Re: [PATCH] i2c: imx: Defer probing if EDMA not available 2019-12-11 11:42 ` Russell King - ARM Linux admin @ 2019-12-12 3:09 ` Peng Ma 2019-12-12 10:58 ` Russell King - ARM Linux admin 0 siblings, 1 reply; 12+ messages in thread From: Peng Ma @ 2019-12-12 3:09 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: shawnguo, s.hauer, linux-kernel, linux, Abel Vesa, Aisheng Dong, Anson Huang, Bogdan Florin Vlad, BOUGH CHEN, Clark Wang, Daniel Baluta, Fancy Fang, Han Xu, Horia Geanta, Iuliana Prodan, Jacky Bai, Joakim Zhang, Jun Li, Leo Zhang, Leonard Crestez, Mircea Pop, Mirela Rabulea, Peng Fan, Peter Chen, Ranjani Vaidyanathan, Robert Chiras, Robin Gong, Shenwei Wang, Viorel Suman, Ying Liu, Zening Wang, kernel, festevam, linux-arm-kernel, linux-i2c 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 <linux@armlinux.org.uk> >Sent: 2019年12月11日 19:43 >To: Peng Ma <peng.ma@nxp.com> >Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; >linux-kernel@vger.kernel.org; linux@rempel-privat.de; dl-linux-imx ><linux-imx@nxp.com>; kernel@pengutronix.de; festevam@gmail.com; >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 11:22:00AM +0000, Peng Ma wrote: >> >> >> >-----Original Message----- >> >From: Russell King - ARM Linux admin <linux@armlinux.org.uk> >> >Sent: 2019年12月11日 18:44 >> >To: Peng Ma <peng.ma@nxp.com> >> >Cc: festevam@gmail.com; s.hauer@pengutronix.de; >> >linux-kernel@vger.kernel.org; linux@rempel-privat.de; dl-linux-imx >> ><linux-imx@nxp.com>; 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: >> >> 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@armlinux.org.uk> >> >> >Sent: 2019年11月28日 18:06 >> >> >To: Peng Ma <peng.ma@nxp.com> >> >> >Cc: linux@rempel-privat.de; kernel@pengutronix.de; >> >> >shawnguo@kernel.org; s.hauer@pengutronix.de; >> >> >linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; >> >> >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: >> >> >> 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@nxp.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. > >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&data=02%7C01%7Cpeng.ma >%40nxp.com%7C142e002eba8940b2250b08d77e2f3999%7C686ea1d3bc2b4c >6fa92cd99c5c301635%7C0%7C0%7C637116613612463295&sdata=AdIo >q6rNyPDcKAjq%2FikDhE8vp3OpyGOUV108TO6r4jo%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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [EXT] Re: [PATCH] i2c: imx: Defer probing if EDMA not available 2019-12-12 3:09 ` Peng Ma @ 2019-12-12 10:58 ` Russell King - ARM Linux admin 2019-12-13 10:33 ` Peng Ma 0 siblings, 1 reply; 12+ messages in thread From: Russell King - ARM Linux admin @ 2019-12-12 10:58 UTC (permalink / raw) To: Peng Ma Cc: shawnguo, s.hauer, linux-kernel, linux, Abel Vesa, Aisheng Dong, Anson Huang, Bogdan Florin Vlad, BOUGH CHEN, Clark Wang, Daniel Baluta, Fancy Fang, Han Xu, Horia Geanta, Iuliana Prodan, Jacky Bai, Joakim Zhang, Jun Li, Leo Zhang, Leonard Crestez, Mircea Pop, Mirela Rabulea, Peng Fan, Peter Chen, Ranjani Vaidyanathan, Robert Chiras, Robin Gong, Shenwei Wang, Viorel Suman, Ying Liu, Zening Wang, kernel, festevam, linux-arm-kernel, linux-i2c 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. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [EXT] Re: [PATCH] i2c: imx: Defer probing if EDMA not available 2019-12-12 10:58 ` Russell King - ARM Linux admin @ 2019-12-13 10:33 ` Peng Ma 2019-12-13 10:50 ` Russell King - ARM Linux admin 0 siblings, 1 reply; 12+ messages in thread From: Peng Ma @ 2019-12-13 10:33 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: shawnguo, s.hauer, linux-kernel, linux, Abel Vesa, Aisheng Dong, Anson Huang, Bogdan Florin Vlad, BOUGH CHEN, Clark Wang, Daniel Baluta, Fancy Fang, Han Xu, Horia Geanta, Iuliana Prodan, Jacky Bai, Joakim Zhang, Jun Li, Leo Zhang, Leonard Crestez, Mircea Pop, Mirela Rabulea, Peng Fan, Peter Chen, Ranjani Vaidyanathan, Robert Chiras, Robin Gong, Shenwei Wang, Viorel Suman, Ying Liu, Zening Wang, kernel, festevam, linux-arm-kernel, linux-i2c >-----Original Message----- >From: Russell King - ARM Linux admin <linux@armlinux.org.uk> >Sent: 2019年12月12日 18:59 >To: Peng Ma <peng.ma@nxp.com> >Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; >linux-kernel@vger.kernel.org; linux@rempel-privat.de; Abel Vesa ><abel.vesa@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>; Anson Huang ><anson.huang@nxp.com>; Bogdan Florin Vlad <bogdan.vlad@nxp.com>; >BOUGH CHEN <haibo.chen@nxp.com>; Clark Wang ><xiaoning.wang@nxp.com>; Daniel Baluta <daniel.baluta@nxp.com>; Fancy >Fang <chen.fang@nxp.com>; Han Xu <han.xu@nxp.com>; Horia Geanta ><horia.geanta@nxp.com>; Iuliana Prodan <iuliana.prodan@nxp.com>; Jacky >Bai <ping.bai@nxp.com>; Joakim Zhang <qiangqing.zhang@nxp.com>; Jun Li ><jun.li@nxp.com>; Leo Zhang <leo.zhang@nxp.com>; Leonard Crestez ><leonard.crestez@nxp.com>; Mircea Pop <mircea.pop@nxp.com>; Mirela >Rabulea <mirela.rabulea@nxp.com>; Peng Fan <peng.fan@nxp.com>; Peter >Chen <peter.chen@nxp.com>; Ranjani Vaidyanathan ><ranjani.vaidyanathan@nxp.com>; Robert Chiras <robert.chiras@nxp.com>; >Robin Gong <yibin.gong@nxp.com>; Shenwei Wang ><shenwei.wang@nxp.com>; Viorel Suman <viorel.suman@nxp.com>; Ying Liu ><victor.liu@nxp.com>; Zening Wang <zening.wang@nxp.com>; >kernel@pengutronix.de; festevam@gmail.com; >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 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&data=02%7C01%7Cpeng.ma >%40nxp.com%7Ca535712eabe343a51c2f08d77ef250e4%7C686ea1d3bc2b4c6 >fa92cd99c5c301635%7C0%7C0%7C637117451627341155&sdata=%2Fyz >xAI8%2FezVRwGTaT4vYa3CMkIMsaSYiaH8DjvJWUKA%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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [EXT] Re: [PATCH] i2c: imx: Defer probing if EDMA not available 2019-12-13 10:33 ` Peng Ma @ 2019-12-13 10:50 ` Russell King - ARM Linux admin 0 siblings, 0 replies; 12+ messages in thread From: Russell King - ARM Linux admin @ 2019-12-13 10:50 UTC (permalink / raw) To: Peng Ma Cc: shawnguo, s.hauer, linux-kernel, linux, Abel Vesa, Aisheng Dong, Anson Huang, Bogdan Florin Vlad, BOUGH CHEN, Clark Wang, Daniel Baluta, Fancy Fang, Han Xu, Horia Geanta, Iuliana Prodan, Jacky Bai, Joakim Zhang, Jun Li, Leo Zhang, Leonard Crestez, Mircea Pop, Mirela Rabulea, Peng Fan, Peter Chen, Ranjani Vaidyanathan, Robert Chiras, Robin Gong, Shenwei Wang, Viorel Suman, Ying Liu, Zening Wang, kernel, festevam, linux-arm-kernel, linux-i2c On Fri, Dec 13, 2019 at 10:33:51AM +0000, Peng Ma wrote: > > > >-----Original Message----- > >From: Russell King - ARM Linux admin <linux@armlinux.org.uk> > >Sent: 2019年12月12日 18:59 > >To: Peng Ma <peng.ma@nxp.com> > >Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; > >linux-kernel@vger.kernel.org; linux@rempel-privat.de; Abel Vesa > ><abel.vesa@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>; Anson Huang > ><anson.huang@nxp.com>; Bogdan Florin Vlad <bogdan.vlad@nxp.com>; > >BOUGH CHEN <haibo.chen@nxp.com>; Clark Wang > ><xiaoning.wang@nxp.com>; Daniel Baluta <daniel.baluta@nxp.com>; Fancy > >Fang <chen.fang@nxp.com>; Han Xu <han.xu@nxp.com>; Horia Geanta > ><horia.geanta@nxp.com>; Iuliana Prodan <iuliana.prodan@nxp.com>; Jacky > >Bai <ping.bai@nxp.com>; Joakim Zhang <qiangqing.zhang@nxp.com>; Jun Li > ><jun.li@nxp.com>; Leo Zhang <leo.zhang@nxp.com>; Leonard Crestez > ><leonard.crestez@nxp.com>; Mircea Pop <mircea.pop@nxp.com>; Mirela > >Rabulea <mirela.rabulea@nxp.com>; Peng Fan <peng.fan@nxp.com>; Peter > >Chen <peter.chen@nxp.com>; Ranjani Vaidyanathan > ><ranjani.vaidyanathan@nxp.com>; Robert Chiras <robert.chiras@nxp.com>; > >Robin Gong <yibin.gong@nxp.com>; Shenwei Wang > ><shenwei.wang@nxp.com>; Viorel Suman <viorel.suman@nxp.com>; Ying Liu > ><victor.liu@nxp.com>; Zening Wang <zening.wang@nxp.com>; > >kernel@pengutronix.de; festevam@gmail.com; > >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 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. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-12-13 10:51 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-27 7:12 [PATCH] i2c: imx: Defer probing if EDMA not available Peng Ma 2019-11-27 7:27 ` Uwe Kleine-König 2019-11-28 2:45 ` [EXT] " Peng Ma 2019-11-28 10:06 ` Russell King - ARM Linux admin 2019-12-11 10:25 ` [EXT] " Peng Ma 2019-12-11 10:43 ` Russell King - ARM Linux admin 2019-12-11 11:22 ` Peng Ma 2019-12-11 11:42 ` Russell King - ARM Linux admin 2019-12-12 3:09 ` Peng Ma 2019-12-12 10:58 ` Russell King - ARM Linux admin 2019-12-13 10:33 ` Peng Ma 2019-12-13 10:50 ` Russell King - ARM Linux admin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).