linux-kernel - RE: [PATCH v5 2/2] i2c: aspeed: support ast2600 i2cv2 new register mode driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <SEZPR06MB526909B8D4313CA5F33074C5F2AB9@SEZPR06MB5269.apcprd06.prod.outlook.com>
Date:   Thu, 23 Feb 2023 00:58:14 +0000
From:   Ryan Chen <ryan_chen@...eedtech.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Joel Stanley <joel@....id.au>,
        Andrew Jeffery <andrew@...id.au>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        "openbmc@...ts.ozlabs.org" <openbmc@...ts.ozlabs.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v5 2/2] i2c: aspeed: support ast2600 i2cv2 new register
 mode driver

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> Sent: Wednesday, February 22, 2023 4:28 PM
> To: Ryan Chen <ryan_chen@...eedtech.com>; Rob Herring
> <robh+dt@...nel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@...aro.org>; Joel Stanley <joel@....id.au>; Andrew
> Jeffery <andrew@...id.au>; Philipp Zabel <p.zabel@...gutronix.de>;
> openbmc@...ts.ozlabs.org; linux-arm-kernel@...ts.infradead.org;
> linux-aspeed@...ts.ozlabs.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v5 2/2] i2c: aspeed: support ast2600 i2cv2 new register
> mode driver
> 
> On 22/02/2023 04:36, Ryan Chen wrote:
> 
> >>> +
> >>> +	return 0;
> >>> +
> >>> +free_irq:
> >>> +	devm_free_irq(&pdev->dev, i2c_bus->irq, i2c_bus);
> >>
> >> Why?
> >>
> >>> +unmap:
> >>> +	devm_iounmap(&pdev->dev, i2c_bus->reg_base);
> >>
> >> Why?
> >>
> >>> +free_mem:
> >>> +	devm_kfree(&pdev->dev, i2c_bus);
> >>
> >> Why?
> >>
> >
> > Sorry, those are probe following, if any error, will goto this label.
> > To release mem/unmap/free_irq. Is this unnecessary?
> 
> Releasing managed resources is usualyl unnecessary. Therefore I am asking
> why do you think it is necessary here?
> 
> > I saw many driver submit is remove all probe fail goto label, is just return
> ERR.
> > Do you mean I direct go for this way?
> 
> Why would you do it differently?

Thanks, I will remove those labels, and modify to dev_err_probe in previous probe return.

> >
> >>> +
> >>> +	return ret;
> >>> +}
> >>> +
> >>> +static int ast2600_i2c_remove(struct platform_device *pdev) {
> >>> +	struct ast2600_i2c_bus *i2c_bus = platform_get_drvdata(pdev);
> >>> +
> >>> +	/* Disable everything. */
> >>> +	writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> >>> +	writel(0, i2c_bus->reg_base + AST2600_I2CM_IER);
> >>> +
> >>> +	devm_free_irq(&pdev->dev, i2c_bus->irq, i2c_bus);
> >>> +
> >>> +	i2c_del_adapter(&i2c_bus->adap);
> >>
> >> Wrong order of cleanup. It should be reversed to the probe, unless
> >> you have some reason, but then please explain.
> >
> > Sorry, this in remove function, it should do i2c_del_adapter(&i2c_bus->adap)
> in the end.
> > Why this should revered to probe?
> 
> Because it's logical, you do the same with error paths of probe, it it usually the
> only way to make sure some of the resources are not used by some other piece
> (e.g. interrupt handler is called while i2c adapter is unregistered).

Sorry, I can't catch your point.
Do you mean remove devm_free_irq(&pdev->dev, i2c_bus->irq, i2c_bus);
Keep i2c_del_adapter(&i2c_bus->adap) here, because interrupt is called while i2c adapter is unregistered ?

Best regards,
Ryan Chen