linux-kernel - RE: [PATCH v4 2/3] hwrng: starfive - Add TRNG driver for StarFive SoC

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] [day] [month] [year] [list]

Message-ID: <82979ef41b4a4c9f8a77c950e3c65c86@EXMBX168.cuchost.com>
Date:   Fri, 13 Jan 2023 02:30:55 +0000
From:   JiaJie Ho <jiajie.ho@...rfivetech.com>
To:     Conor Dooley <conor@...nel.org>
CC:     Olivia Mackall <olivia@...enic.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Rob Herring <robh+dt@...nel.org>,
        "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@...aro.org>,
        Emil Renner Berthing <kernel@...il.dk>,
        Conor Dooley <conor.dooley@...rochip.com>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>
Subject: RE: [PATCH v4 2/3] hwrng: starfive - Add TRNG driver for StarFive SoC

> > +STARFIVE TRNG DRIVER
> > +M:	Jia Jie Ho <jiajie.ho@...rfivetech.com>
> > +S:	Maintained
> > +F:	Documentation/devicetree/bindings/rng/starfive*
> > +F:	drivers/char/hw_random/starfive-trng.c
> 
> minor nit (so don't submit another version just to fix this):
> This should be Supported, no?
> 

Hi Conor, 
I'll update this in next version together with other comments below. 

> > diff --git a/drivers/char/hw_random/Makefile
> > b/drivers/char/hw_random/Makefile index 3e948cf04476..f68ac370847f
> > 100644
> > --- a/drivers/char/hw_random/Makefile
> > +++ b/drivers/char/hw_random/Makefile
> > @@ -47,3 +47,4 @@ obj-$(CONFIG_HW_RANDOM_XIPHERA) += xiphera-
> trng.o
> >  obj-$(CONFIG_HW_RANDOM_ARM_SMCCC_TRNG) += arm_smccc_trng.o
> >  obj-$(CONFIG_HW_RANDOM_CN10K) += cn10k-rng.o
> >  obj-$(CONFIG_HW_RANDOM_POLARFIRE_SOC) += mpfs-rng.o
> > +obj-$(CONFIG_HW_RANDOM_STARFIVE) += starfive-trng.o
> 
> Is "STARFIVE" a bit too general of a name here and in the Kconfig entry?
> I don't have a TRM for the JH7100, but this name (and the Kconfig text)
> would give me the impression that I can use it there too.
> Does this driver support both?
> 

7100 uses a different trng module but this same generator might be used in
future products, so I left it generic. Would it be better to specify the product?

> > +static int starfive_trng_probe(struct platform_device *pdev)
[...]
> > +	ret = devm_hwrng_register(&pdev->dev, &trng->rng);
> > +	if (ret) {
> > +		dev_err_probe(&pdev->dev, ret, "Failed to register
> hwrng\n");
> > +		goto err_fail_register;
> > +	}
> > +
> > +	pm_runtime_use_autosuspend(&pdev->dev);
> > +	pm_runtime_set_autosuspend_delay(&pdev->dev, 100);
> 
> > +	pm_runtime_enable(&pdev->dev);
> 
> > +
> > +	return 0;
> > +
> > +err_fail_register:
> 
> > +	pm_runtime_disable(&pdev->dev);
> 
> This was only enabled after the only goto for this label, does it serve a
> purpose?
> I know little about runtime PM, it just caught my eye.
> I looked at the other rng drivers that had calls to pm_runtime_enable(), but
> they all seem to do their pm enablement _before_ calling hwrng_register().
> Again, I am not familiar with runtime PM, but curious why you are doing
> things differently, that's all.

It does make more sense to move pm_runtime_enable before registering the
generator to align with codes in the goto label.
I'll fix this part.

Thanks again for reviewing the patches.

Best regards,
Jia Jie