| Message ID | 20200908224006.25636-15-digetx@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Improvements for Tegra I2C driver | expand |
| Context | Check | Description |
|---|---|---|
| tagr/GVS | success | 1124251 |
| tagr/GVS-1122491 | pending | None |
| tagr/GVS-1122491 | fail | None |
| tagr/GVS | pending | 1122533 |
| tagr/GVS | success | 1122533 |
| tagr/GVS | pending | 1124251 |
On Wed, Sep 09, 2020 at 01:39:46AM +0300, Dmitry Osipenko wrote: > The driver's probe function code is a bit difficult to read. This patch > reorders code of the probe function, forming groups of code that are easy > to work with. The probe tear-down order now matches the driver-removal > order. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 100 ++++++++++++++++----------------- > 1 file changed, 49 insertions(+), 51 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index 79d1cefdc901..7c91bbb3f95c 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -440,6 +440,9 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev) > > i2c_dev->tx_dma_chan = chan; > > + i2c_dev->dma_buf_size = i2c_dev->hw->quirks->max_write_len + > + I2C_PACKET_HEADER_SIZE; > + > dma_buf = dma_alloc_coherent(i2c_dev->dev, i2c_dev->dma_buf_size, > &dma_phys, GFP_KERNEL | __GFP_NOWARN); > if (!dma_buf) { > @@ -1693,34 +1696,42 @@ static int tegra_i2c_probe(struct platform_device *pdev) > struct device *dev = &pdev->dev; > struct tegra_i2c_dev *i2c_dev; > struct resource *res; > - void __iomem *base; > - phys_addr_t base_phys; > - int irq; > int ret; > > - base = devm_platform_get_and_ioremap_resource(pdev, 0, &res); > - if (IS_ERR(base)) > - return PTR_ERR(base); > - > - base_phys = res->start; > - > - irq = platform_get_irq(pdev, 0); > - if (irq < 0) > - return irq; > - > i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL); > if (!i2c_dev) > return -ENOMEM; > > - i2c_dev->base = base; > - i2c_dev->base_phys = base_phys; > - i2c_dev->adapter.algo = &tegra_i2c_algo; > - i2c_dev->adapter.retries = 1; > - i2c_dev->adapter.timeout = 6 * HZ; > - i2c_dev->irq = irq; > + platform_set_drvdata(pdev, i2c_dev); > + > + init_completion(&i2c_dev->msg_complete); > + init_completion(&i2c_dev->dma_complete); > + > + i2c_dev->hw = of_device_get_match_data(&pdev->dev); > i2c_dev->cont_id = pdev->id; > i2c_dev->dev = &pdev->dev; > > + i2c_dev->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res); > + if (IS_ERR(i2c_dev->base)) > + return PTR_ERR(i2c_dev->base); > + > + i2c_dev->base_phys = res->start; > + > + ret = platform_get_irq(pdev, 0); > + if (ret < 0) > + return ret; > + > + i2c_dev->irq = ret; > + > + /* interrupt will be enabled during of transfer time */ > + irq_set_status_flags(i2c_dev->irq, IRQ_NOAUTOEN); > + > + ret = devm_request_irq(&pdev->dev, i2c_dev->irq, tegra_i2c_isr, > + IRQF_NO_SUSPEND, dev_name(&pdev->dev), > + i2c_dev); > + if (ret) > + return ret; Is it safe to install the interrupt handler at this point? What if, perhaps because some bootloader didn't properly quiesce the I2C controller, an interrupt triggers immediately after this? Thierry
On Thu, Sep 17, 2020 at 3:37 PM Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Sep 09, 2020 at 01:39:46AM +0300, Dmitry Osipenko wrote: ... > > + ret = devm_request_irq(&pdev->dev, i2c_dev->irq, tegra_i2c_isr, > > + IRQF_NO_SUSPEND, dev_name(&pdev->dev), > > + i2c_dev); > > + if (ret) > > + return ret; > > Is it safe to install the interrupt handler at this point? What if, > perhaps because some bootloader didn't properly quiesce the I2C > controller, an interrupt triggers immediately after this? It\s easy to check with debug shared IRQ, but here IRQ is not shared... So, with a hack into the code and enabled debug it will be achievable to test. And you probably meant that IRQ triggers *before* the handler is in place?
17.09.2020 15:37, Thierry Reding пишет: ... >> + /* interrupt will be enabled during of transfer time */ >> + irq_set_status_flags(i2c_dev->irq, IRQ_NOAUTOEN); ^^^^^^^^^^^^ >> + ret = devm_request_irq(&pdev->dev, i2c_dev->irq, tegra_i2c_isr, >> + IRQF_NO_SUSPEND, dev_name(&pdev->dev), >> + i2c_dev); >> + if (ret) >> + return ret; > > Is it safe to install the interrupt handler at this point? What if, > perhaps because some bootloader didn't properly quiesce the I2C > controller, an interrupt triggers immediately after this? This is why we're using the IRQ_NOAUTOEN flag above :)
On Wed, 09 Sep 2020 01:39:46 +0300, Dmitry Osipenko wrote: > The driver's probe function code is a bit difficult to read. This patch > reorders code of the probe function, forming groups of code that are easy > to work with. The probe tear-down order now matches the driver-removal > order. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 100 ++++++++++++++++----------------- > 1 file changed, 49 insertions(+), 51 deletions(-) Tested-by: Thierry Reding <treding@nvidia.com>
On Thu, Sep 17, 2020 at 04:46:45PM +0300, Andy Shevchenko wrote: > On Thu, Sep 17, 2020 at 3:37 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Wed, Sep 09, 2020 at 01:39:46AM +0300, Dmitry Osipenko wrote: > > ... > > > > + ret = devm_request_irq(&pdev->dev, i2c_dev->irq, tegra_i2c_isr, > > > + IRQF_NO_SUSPEND, dev_name(&pdev->dev), > > > + i2c_dev); > > > + if (ret) > > > + return ret; > > > > Is it safe to install the interrupt handler at this point? What if, > > perhaps because some bootloader didn't properly quiesce the I2C > > controller, an interrupt triggers immediately after this? > > It\s easy to check with debug shared IRQ, but here IRQ is not shared... > So, with a hack into the code and enabled debug it will be achievable to test. > > And you probably meant that IRQ triggers *before* the handler is in place? It shouldn't be possible for the interrupt to trigger before the handler is in place, because requesting the IRQ here is what will unmask the IRQ at the parent. I'm more concerned that the hardware may be in some state where it already has a pending interrupt and therefore unmasking (as part of the request here) is going to immediately trigger an interrupt. If we haven't set up i2c_dev completely at this point this may cause issues because the interrupt handler will now have to deal with a partially initialized structure. Thierry
On Thu, Sep 17, 2020 at 06:02:26PM +0300, Dmitry Osipenko wrote: > 17.09.2020 15:37, Thierry Reding пишет: > ... > >> + /* interrupt will be enabled during of transfer time */ > >> + irq_set_status_flags(i2c_dev->irq, IRQ_NOAUTOEN); > ^^^^^^^^^^^^ > > >> + ret = devm_request_irq(&pdev->dev, i2c_dev->irq, tegra_i2c_isr, > >> + IRQF_NO_SUSPEND, dev_name(&pdev->dev), > >> + i2c_dev); > >> + if (ret) > >> + return ret; > > > > Is it safe to install the interrupt handler at this point? What if, > > perhaps because some bootloader didn't properly quiesce the I2C > > controller, an interrupt triggers immediately after this? > > This is why we're using the IRQ_NOAUTOEN flag above :) Ah, I missed that. Seems fine then: Reviewed-by: Thierry Reding <treding@nvidia.com>
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index 79d1cefdc901..7c91bbb3f95c 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -440,6 +440,9 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev) i2c_dev->tx_dma_chan = chan; + i2c_dev->dma_buf_size = i2c_dev->hw->quirks->max_write_len + + I2C_PACKET_HEADER_SIZE; + dma_buf = dma_alloc_coherent(i2c_dev->dev, i2c_dev->dma_buf_size, &dma_phys, GFP_KERNEL | __GFP_NOWARN); if (!dma_buf) { @@ -1693,34 +1696,42 @@ static int tegra_i2c_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct tegra_i2c_dev *i2c_dev; struct resource *res; - void __iomem *base; - phys_addr_t base_phys; - int irq; int ret; - base = devm_platform_get_and_ioremap_resource(pdev, 0, &res); - if (IS_ERR(base)) - return PTR_ERR(base); - - base_phys = res->start; - - irq = platform_get_irq(pdev, 0); - if (irq < 0) - return irq; - i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL); if (!i2c_dev) return -ENOMEM; - i2c_dev->base = base; - i2c_dev->base_phys = base_phys; - i2c_dev->adapter.algo = &tegra_i2c_algo; - i2c_dev->adapter.retries = 1; - i2c_dev->adapter.timeout = 6 * HZ; - i2c_dev->irq = irq; + platform_set_drvdata(pdev, i2c_dev); + + init_completion(&i2c_dev->msg_complete); + init_completion(&i2c_dev->dma_complete); + + i2c_dev->hw = of_device_get_match_data(&pdev->dev); i2c_dev->cont_id = pdev->id; i2c_dev->dev = &pdev->dev; + i2c_dev->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res); + if (IS_ERR(i2c_dev->base)) + return PTR_ERR(i2c_dev->base); + + i2c_dev->base_phys = res->start; + + ret = platform_get_irq(pdev, 0); + if (ret < 0) + return ret; + + i2c_dev->irq = ret; + + /* interrupt will be enabled during of transfer time */ + irq_set_status_flags(i2c_dev->irq, IRQ_NOAUTOEN); + + ret = devm_request_irq(&pdev->dev, i2c_dev->irq, tegra_i2c_isr, + IRQF_NO_SUSPEND, dev_name(&pdev->dev), + i2c_dev); + if (ret) + return ret; + i2c_dev->rst = devm_reset_control_get_exclusive(&pdev->dev, "i2c"); if (IS_ERR(i2c_dev->rst)) { dev_err_probe(&pdev->dev, PTR_ERR(i2c_dev->rst), @@ -1734,14 +1745,9 @@ static int tegra_i2c_probe(struct platform_device *pdev) if (ret) return ret; - i2c_dev->hw = of_device_get_match_data(&pdev->dev); - i2c_dev->adapter.quirks = i2c_dev->hw->quirks; - i2c_dev->dma_buf_size = i2c_dev->adapter.quirks->max_write_len + - I2C_PACKET_HEADER_SIZE; - init_completion(&i2c_dev->msg_complete); - init_completion(&i2c_dev->dma_complete); - - platform_set_drvdata(pdev, i2c_dev); + ret = tegra_i2c_init_dma(i2c_dev); + if (ret) + goto release_clocks; /* * VI I2C is in VE power domain which is not always on and not @@ -1759,49 +1765,41 @@ static int tegra_i2c_probe(struct platform_device *pdev) goto put_rpm; } - if (i2c_dev->hw->supports_bus_clear) - i2c_dev->adapter.bus_recovery_info = &tegra_i2c_recovery_info; - - ret = tegra_i2c_init_dma(i2c_dev); - if (ret < 0) - goto put_rpm; - ret = tegra_i2c_init(i2c_dev); - if (ret) { - dev_err(&pdev->dev, "Failed to initialize i2c controller\n"); - goto release_dma; - } - - irq_set_status_flags(i2c_dev->irq, IRQ_NOAUTOEN); - - ret = devm_request_irq(&pdev->dev, i2c_dev->irq, tegra_i2c_isr, - IRQF_NO_SUSPEND, dev_name(&pdev->dev), i2c_dev); if (ret) - goto release_dma; + goto put_rpm; i2c_set_adapdata(&i2c_dev->adapter, i2c_dev); + i2c_dev->adapter.dev.of_node = pdev->dev.of_node; + i2c_dev->adapter.dev.parent = &pdev->dev; + i2c_dev->adapter.retries = 1; + i2c_dev->adapter.timeout = 6 * HZ; + i2c_dev->adapter.quirks = i2c_dev->hw->quirks; i2c_dev->adapter.owner = THIS_MODULE; i2c_dev->adapter.class = I2C_CLASS_DEPRECATED; + i2c_dev->adapter.algo = &tegra_i2c_algo; + i2c_dev->adapter.nr = pdev->id; + + if (i2c_dev->hw->supports_bus_clear) + i2c_dev->adapter.bus_recovery_info = &tegra_i2c_recovery_info; + strlcpy(i2c_dev->adapter.name, dev_name(&pdev->dev), sizeof(i2c_dev->adapter.name)); - i2c_dev->adapter.dev.parent = &pdev->dev; - i2c_dev->adapter.nr = pdev->id; - i2c_dev->adapter.dev.of_node = pdev->dev.of_node; ret = i2c_add_numbered_adapter(&i2c_dev->adapter); if (ret) - goto release_dma; + goto put_rpm; pm_runtime_put(&pdev->dev); return 0; -release_dma: - tegra_i2c_release_dma(i2c_dev); - put_rpm: pm_runtime_put_sync(&pdev->dev); pm_runtime_disable(&pdev->dev); + + tegra_i2c_release_dma(i2c_dev); +release_clocks: tegra_i2c_release_clocks(i2c_dev); return ret;
The driver's probe function code is a bit difficult to read. This patch reorders code of the probe function, forming groups of code that are easy to work with. The probe tear-down order now matches the driver-removal order. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/i2c/busses/i2c-tegra.c | 100 ++++++++++++++++----------------- 1 file changed, 49 insertions(+), 51 deletions(-)