| Message ID | 20241210231054.2844202-3-andi.shyti@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Qcom Geni exit path cleanups | expand |
Thanks Andi for this change ! On 12/11/2024 4:40 AM, Andi Shyti wrote: > Avoid repeating the error handling pattern: > > geni_se_resources_off(&gi2c->se); > clk_disable_unprepare(gi2c->core_clk); > return; > > Introduce a single 'goto' exit label for cleanup in case of > errors. While there are currently two distinct exit points, there > is no overlap in their handling, allowing both branches to > coexist cleanly. > > Signed-off-by: Andi Shyti <andi.shyti@kernel.org> > --- > drivers/i2c/busses/i2c-qcom-geni.c | 30 ++++++++++++++++-------------- > 1 file changed, 16 insertions(+), 14 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c > index 01db24188e29..3fc85595a4aa 100644 > --- a/drivers/i2c/busses/i2c-qcom-geni.c > +++ b/drivers/i2c/busses/i2c-qcom-geni.c > @@ -867,14 +867,13 @@ static int geni_i2c_probe(struct platform_device *pdev) > > ret = geni_se_resources_on(&gi2c->se); > if (ret) { > - clk_disable_unprepare(gi2c->core_clk); > - return dev_err_probe(dev, ret, "Error turning on resources\n"); > + dev_err_probe(dev, ret, "Error turning on resources\n"); > + goto err_clk; > } > proto = geni_se_read_proto(&gi2c->se); > if (proto != GENI_SE_I2C) { > - geni_se_resources_off(&gi2c->se); > - clk_disable_unprepare(gi2c->core_clk); > - return dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto); > + dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto); Suggestive comment, can we make this second patch as first patch ? So that we can have both above lines reduced in this patch. > + goto err_off; > } > > if (desc && desc->no_dma_support) > @@ -886,11 +885,8 @@ static int geni_i2c_probe(struct platform_device *pdev) > /* FIFO is disabled, so we can only use GPI DMA */ > gi2c->gpi_mode = true; > ret = setup_gpi_dma(gi2c); > - if (ret) { > - geni_se_resources_off(&gi2c->se); > - clk_disable_unprepare(gi2c->core_clk); > - return ret; > - } > + if (ret) > + goto err_off; > > dev_dbg(dev, "Using GPI DMA mode for I2C\n"); > } else { > @@ -902,10 +898,8 @@ static int geni_i2c_probe(struct platform_device *pdev) > tx_depth = desc->tx_fifo_depth; > > if (!tx_depth) { > - geni_se_resources_off(&gi2c->se); > - clk_disable_unprepare(gi2c->core_clk); > - return dev_err_probe(dev, -EINVAL, > - "Invalid TX FIFO depth\n"); > + dev_err_probe(dev, -EINVAL, "Invalid TX FIFO depth\n"); > + goto err_off; > } > > gi2c->tx_wm = tx_depth - 1; > @@ -944,8 +938,16 @@ static int geni_i2c_probe(struct platform_device *pdev) > > return 0; return ret here ? yes, we need to initialize ret = 0. > > +err_off: can we rename as err_resources ? > + geni_se_resources_off(&gi2c->se); > +err_clk: > + clk_disable_unprepare(gi2c->core_clk); > + > + return ret; > + > err_dma: > release_gpi_dma(gi2c); > + > return ret; > } >
Hi Mukesh, On Wed, Dec 11, 2024 at 09:26:53AM +0530, Mukesh Kumar Savaliya wrote: > Thanks Andi for this change ! Thanks for looking into it. ... > > @@ -944,8 +938,16 @@ static int geni_i2c_probe(struct platform_device *pdev) > > return 0; > return ret here ? yes, we need to initialize ret = 0. here? It's returning '0', as it was before. I'm failing to see where 'ret' is used uninitialized. What am I missing? > > +err_off: > can we rename as err_resources ? yes, it's better, as meaning it's more aligned with the other labels. Thanks, Andi > > + geni_se_resources_off(&gi2c->se); > > +err_clk: > > + clk_disable_unprepare(gi2c->core_clk); > > + > > + return ret; > > + > > err_dma: > > release_gpi_dma(gi2c); > > + > > return ret; > > } >
Thanks Andi ! On 12/11/2024 12:26 PM, Andi Shyti wrote: > Hi Mukesh, > > On Wed, Dec 11, 2024 at 09:26:53AM +0530, Mukesh Kumar Savaliya wrote: >> Thanks Andi for this change ! > > Thanks for looking into it. > > ... > >>> @@ -944,8 +938,16 @@ static int geni_i2c_probe(struct platform_device *pdev) >>> return 0; >> return ret here ? yes, we need to initialize ret = 0. > > here? It's returning '0', as it was before. I'm failing to see > where 'ret' is used uninitialized. What am I missing? > My point is - Except this place, rest of the places we are returning ret OR standard error code. If we return as ret with initializing 0 at the start of probe function, it would look good. As such No strict requirement. >>> +err_off: >> can we rename as err_resources ? > > yes, it's better, as meaning it's more aligned with the other > labels. > > Thanks, > Andi > >>> + geni_se_resources_off(&gi2c->se); >>> +err_clk: >>> + clk_disable_unprepare(gi2c->core_clk); >>> + >>> + return ret; >>> + >>> err_dma: >>> release_gpi_dma(gi2c); >>> + >>> return ret; >>> } >>
> > > > @@ -944,8 +938,16 @@ static int geni_i2c_probe(struct platform_device *pdev) > > > > return 0; > > > return ret here ? yes, we need to initialize ret = 0. > > > > here? It's returning '0', as it was before. I'm failing to see > > where 'ret' is used uninitialized. What am I missing? > > > My point is - Except this place, rest of the places we are returning ret OR > standard error code. If we return as ret with initializing 0 at the start of > probe function, it would look good. As such No strict requirement. Ah, I see. Sure, I can do a "return ret" in my v2. Thanks, Andi
Hi Mukesh, > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c > > index 01db24188e29..3fc85595a4aa 100644 > > --- a/drivers/i2c/busses/i2c-qcom-geni.c > > +++ b/drivers/i2c/busses/i2c-qcom-geni.c > > @@ -867,14 +867,13 @@ static int geni_i2c_probe(struct platform_device *pdev) > > ret = geni_se_resources_on(&gi2c->se); > > if (ret) { > > - clk_disable_unprepare(gi2c->core_clk); > > - return dev_err_probe(dev, ret, "Error turning on resources\n"); > > + dev_err_probe(dev, ret, "Error turning on resources\n"); > > + goto err_clk; > > } > > proto = geni_se_read_proto(&gi2c->se); > > if (proto != GENI_SE_I2C) { > > - geni_se_resources_off(&gi2c->se); > > - clk_disable_unprepare(gi2c->core_clk); > > - return dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto); > > + dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto); > Suggestive comment, can we make this second patch as first patch ? So that > we can have both above lines reduced in this patch. I'm sorry, I missed this comment. I tried to swap it and there is not much reduction in line changes[*]. The reason I chose this order is to avoid changing the same line on both the patches, like here[**]. If it's not binding for you, I would keep the ordering. Thanks again a lot for looking into this, Andi [*] https://paste.debian.net/1339486/ [**] https://paste.debian.net/1339488/ > > + goto err_off; > > } > > if (desc && desc->no_dma_support) > > @@ -886,11 +885,8 @@ static int geni_i2c_probe(struct platform_device *pdev) > > /* FIFO is disabled, so we can only use GPI DMA */ > > gi2c->gpi_mode = true; > > ret = setup_gpi_dma(gi2c); > > - if (ret) { > > - geni_se_resources_off(&gi2c->se); > > - clk_disable_unprepare(gi2c->core_clk); > > - return ret; > > - } > > + if (ret) > > + goto err_off; > > dev_dbg(dev, "Using GPI DMA mode for I2C\n"); > > } else { > > @@ -902,10 +898,8 @@ static int geni_i2c_probe(struct platform_device *pdev) > > tx_depth = desc->tx_fifo_depth; > > if (!tx_depth) { > > - geni_se_resources_off(&gi2c->se); > > - clk_disable_unprepare(gi2c->core_clk); > > - return dev_err_probe(dev, -EINVAL, > > - "Invalid TX FIFO depth\n"); > > + dev_err_probe(dev, -EINVAL, "Invalid TX FIFO depth\n"); > > + goto err_off; > > } > > gi2c->tx_wm = tx_depth - 1; > > @@ -944,8 +938,16 @@ static int geni_i2c_probe(struct platform_device *pdev) > > return 0; > return ret here ? yes, we need to initialize ret = 0. > > +err_off: > can we rename as err_resources ? > > + geni_se_resources_off(&gi2c->se); > > +err_clk: > > + clk_disable_unprepare(gi2c->core_clk); > > + > > + return ret; > > + > > err_dma: > > release_gpi_dma(gi2c); > > + > > return ret; > > } >
Thanks Andi ! On 12/12/2024 7:15 PM, Andi Shyti wrote: > Hi Mukesh, > >>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c >>> index 01db24188e29..3fc85595a4aa 100644 >>> --- a/drivers/i2c/busses/i2c-qcom-geni.c >>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c >>> @@ -867,14 +867,13 @@ static int geni_i2c_probe(struct platform_device *pdev) >>> ret = geni_se_resources_on(&gi2c->se); >>> if (ret) { >>> - clk_disable_unprepare(gi2c->core_clk); >>> - return dev_err_probe(dev, ret, "Error turning on resources\n"); >>> + dev_err_probe(dev, ret, "Error turning on resources\n"); >>> + goto err_clk; >>> } >>> proto = geni_se_read_proto(&gi2c->se); >>> if (proto != GENI_SE_I2C) { >>> - geni_se_resources_off(&gi2c->se); >>> - clk_disable_unprepare(gi2c->core_clk); >>> - return dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto); >>> + dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto); >> Suggestive comment, can we make this second patch as first patch ? So that >> we can have both above lines reduced in this patch. > > I'm sorry, I missed this comment. I tried to swap it and there is > not much reduction in line changes[*]. > > The reason I chose this order is to avoid changing the same line > on both the patches, like here[**]. > > If it's not binding for you, I would keep the ordering. > Sure, it's perfectly fine to me. No worries. > Thanks again a lot for looking into this, > Andi > > [*] https://paste.debian.net/1339486/ > [**] https://paste.debian.net/1339488/ > >>> + goto err_off; >>> } >>> if (desc && desc->no_dma_support) >>> @@ -886,11 +885,8 @@ static int geni_i2c_probe(struct platform_device *pdev) >>> /* FIFO is disabled, so we can only use GPI DMA */ >>> gi2c->gpi_mode = true; >>> ret = setup_gpi_dma(gi2c); >>> - if (ret) { >>> - geni_se_resources_off(&gi2c->se); >>> - clk_disable_unprepare(gi2c->core_clk); >>> - return ret; >>> - } >>> + if (ret) >>> + goto err_off; >>> dev_dbg(dev, "Using GPI DMA mode for I2C\n"); >>> } else { >>> @@ -902,10 +898,8 @@ static int geni_i2c_probe(struct platform_device *pdev) >>> tx_depth = desc->tx_fifo_depth; >>> if (!tx_depth) { >>> - geni_se_resources_off(&gi2c->se); >>> - clk_disable_unprepare(gi2c->core_clk); >>> - return dev_err_probe(dev, -EINVAL, >>> - "Invalid TX FIFO depth\n"); >>> + dev_err_probe(dev, -EINVAL, "Invalid TX FIFO depth\n"); >>> + goto err_off; >>> } >>> gi2c->tx_wm = tx_depth - 1; >>> @@ -944,8 +938,16 @@ static int geni_i2c_probe(struct platform_device *pdev) >>> return 0; >> return ret here ? yes, we need to initialize ret = 0. >>> +err_off: >> can we rename as err_resources ? >>> + geni_se_resources_off(&gi2c->se); >>> +err_clk: >>> + clk_disable_unprepare(gi2c->core_clk); >>> + >>> + return ret; >>> + >>> err_dma: >>> release_gpi_dma(gi2c); >>> + >>> return ret; >>> } >>
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c index 01db24188e29..3fc85595a4aa 100644 --- a/drivers/i2c/busses/i2c-qcom-geni.c +++ b/drivers/i2c/busses/i2c-qcom-geni.c @@ -867,14 +867,13 @@ static int geni_i2c_probe(struct platform_device *pdev) ret = geni_se_resources_on(&gi2c->se); if (ret) { - clk_disable_unprepare(gi2c->core_clk); - return dev_err_probe(dev, ret, "Error turning on resources\n"); + dev_err_probe(dev, ret, "Error turning on resources\n"); + goto err_clk; } proto = geni_se_read_proto(&gi2c->se); if (proto != GENI_SE_I2C) { - geni_se_resources_off(&gi2c->se); - clk_disable_unprepare(gi2c->core_clk); - return dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto); + dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto); + goto err_off; } if (desc && desc->no_dma_support) @@ -886,11 +885,8 @@ static int geni_i2c_probe(struct platform_device *pdev) /* FIFO is disabled, so we can only use GPI DMA */ gi2c->gpi_mode = true; ret = setup_gpi_dma(gi2c); - if (ret) { - geni_se_resources_off(&gi2c->se); - clk_disable_unprepare(gi2c->core_clk); - return ret; - } + if (ret) + goto err_off; dev_dbg(dev, "Using GPI DMA mode for I2C\n"); } else { @@ -902,10 +898,8 @@ static int geni_i2c_probe(struct platform_device *pdev) tx_depth = desc->tx_fifo_depth; if (!tx_depth) { - geni_se_resources_off(&gi2c->se); - clk_disable_unprepare(gi2c->core_clk); - return dev_err_probe(dev, -EINVAL, - "Invalid TX FIFO depth\n"); + dev_err_probe(dev, -EINVAL, "Invalid TX FIFO depth\n"); + goto err_off; } gi2c->tx_wm = tx_depth - 1; @@ -944,8 +938,16 @@ static int geni_i2c_probe(struct platform_device *pdev) return 0; +err_off: + geni_se_resources_off(&gi2c->se); +err_clk: + clk_disable_unprepare(gi2c->core_clk); + + return ret; + err_dma: release_gpi_dma(gi2c); + return ret; }
Avoid repeating the error handling pattern: geni_se_resources_off(&gi2c->se); clk_disable_unprepare(gi2c->core_clk); return; Introduce a single 'goto' exit label for cleanup in case of errors. While there are currently two distinct exit points, there is no overlap in their handling, allowing both branches to coexist cleanly. Signed-off-by: Andi Shyti <andi.shyti@kernel.org> --- drivers/i2c/busses/i2c-qcom-geni.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)