diff mbox series

[v7,14/34] i2c: tegra: Clean up probe function

Message ID 20200908224006.25636-15-digetx@gmail.com
State Superseded
Headers show
Series Improvements for Tegra I2C driver | expand

Commit Message

Dmitry Osipenko Sept. 8, 2020, 10:39 p.m. UTC
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(-)

Comments

Thierry Reding Sept. 17, 2020, 12:37 p.m. UTC | #1
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
Andy Shevchenko Sept. 17, 2020, 1:46 p.m. UTC | #2
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?

-- 
With Best Regards,
Andy Shevchenko
Dmitry Osipenko Sept. 17, 2020, 3:02 p.m. UTC | #3
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 :)
Thierry Reding Sept. 21, 2020, 10:19 a.m. UTC | #4
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>
Thierry Reding Sept. 21, 2020, 11:15 a.m. UTC | #5
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
Thierry Reding Sept. 21, 2020, 11:17 a.m. UTC | #6
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 mbox series

Patch

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;