| Message ID | 20250317-plat2faux_dev-v1-2-5fe67c085ad5@arm.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | drivers: Transition to the faux device interface | expand |
On Mon, Mar 17, 2025 at 10:13:14AM +0000, Sudeep Holla wrote:
> +MODULE_ALIAS("faux:smccc_trng");
Why do you need a branch new alias you just made up? Please don't add
that for these types of devices, that's not going to work at all (just
like the platform alias really doesn't work well.
thanks,
greg k-h
On Mon, Mar 17, 2025 at 02:04:27PM +0100, Greg Kroah-Hartman wrote: > On Mon, Mar 17, 2025 at 10:13:14AM +0000, Sudeep Holla wrote: > > +MODULE_ALIAS("faux:smccc_trng"); > > Why do you need a branch new alias you just made up? Please don't add > that for these types of devices, that's not going to work at all (just > like the platform alias really doesn't work well. > Sure I will drop all of those alias. One question I have if the idea of creating a macro for this is good or bad ? I need this initial condition flag to make use of such a macro, so I didn't go for it, but it does remove some boiler-plate code. Let me know what do you think of it ? Regards, Sudeep -->8 diff --git i/include/linux/device/faux.h w/include/linux/device/faux.h index 9f43c0e46aa4..8af3eaef281a 100644 --- i/include/linux/device/faux.h +++ w/include/linux/device/faux.h @@ -66,4 +66,30 @@ static inline void faux_device_set_drvdata(struct faux_device *faux_dev, void *d dev_set_drvdata(&faux_dev->dev, data); } +#define module_faux_driver(name, tag, init_cond) \ +static struct faux_device_ops tag##_ops = { \ + .probe = tag##_probe, \ + .remove = tag##_remove, \ +}; \ + \ +static struct faux_device *tag##_dev; \ + \ +static int __init tag##_init(void) \ +{ \ + if (!(init_cond)) \ + return 0; \ + tag##_dev = faux_device_create(name, NULL, &tag##_ops); \ + if (!tag##_dev) \ + return -ENODEV; \ + \ + return 0; \ +} \ +module_init(tag##_init); \ + \ +static void __exit tag##_exit(void) \ +{ \ + faux_device_destroy(tag##_dev); \ +} \ +module_exit(tag##_exit); \ + #endif /* _FAUX_DEVICE_H_ */
On Mon, Mar 17, 2025 at 02:22:45PM +0000, Sudeep Holla wrote: > On Mon, Mar 17, 2025 at 02:04:27PM +0100, Greg Kroah-Hartman wrote: > > On Mon, Mar 17, 2025 at 10:13:14AM +0000, Sudeep Holla wrote: > > > +MODULE_ALIAS("faux:smccc_trng"); > > > > Why do you need a branch new alias you just made up? Please don't add > > that for these types of devices, that's not going to work at all (just > > like the platform alias really doesn't work well. > > > > Sure I will drop all of those alias. One question I have if the idea of > creating a macro for this is good or bad ? I need this initial condition > flag to make use of such a macro, so I didn't go for it, but it does > remove some boiler-plate code. > > Let me know what do you think of it ? > > Regards, > Sudeep > > -->8 > diff --git i/include/linux/device/faux.h w/include/linux/device/faux.h > index 9f43c0e46aa4..8af3eaef281a 100644 > --- i/include/linux/device/faux.h > +++ w/include/linux/device/faux.h > @@ -66,4 +66,30 @@ static inline void faux_device_set_drvdata(struct faux_device *faux_dev, void *d > dev_set_drvdata(&faux_dev->dev, data); > } > > +#define module_faux_driver(name, tag, init_cond) \ > +static struct faux_device_ops tag##_ops = { \ > + .probe = tag##_probe, \ > + .remove = tag##_remove, \ > +}; \ > + \ > +static struct faux_device *tag##_dev; \ > + \ > +static int __init tag##_init(void) \ > +{ \ > + if (!(init_cond)) \ > + return 0; \ > + tag##_dev = faux_device_create(name, NULL, &tag##_ops); \ > + if (!tag##_dev) \ > + return -ENODEV; \ > + \ > + return 0; \ > +} \ > +module_init(tag##_init); \ > + \ > +static void __exit tag##_exit(void) \ > +{ \ > + faux_device_destroy(tag##_dev); \ > +} \ > +module_exit(tag##_exit); \ Yes, I see that some of your changes could be moved to use this, so I think it is worth it. But why can't you use module_driver() here? Ah, that init_cond? And the device. Hm, why not put the init_cond in the probe callback? That should make this macro simpler. And don't use "tag", that's an odd term here, just copy what module_driver() does instead please. thanks, greg k-h
On Mon, Mar 17, 2025 at 03:30:15PM +0100, Greg Kroah-Hartman wrote: > On Mon, Mar 17, 2025 at 02:22:45PM +0000, Sudeep Holla wrote: > > On Mon, Mar 17, 2025 at 02:04:27PM +0100, Greg Kroah-Hartman wrote: > > > On Mon, Mar 17, 2025 at 10:13:14AM +0000, Sudeep Holla wrote: > > > > +MODULE_ALIAS("faux:smccc_trng"); > > > > > > Why do you need a branch new alias you just made up? Please don't add > > > that for these types of devices, that's not going to work at all (just > > > like the platform alias really doesn't work well. > > > > > > > Sure I will drop all of those alias. One question I have if the idea of > > creating a macro for this is good or bad ? I need this initial condition > > flag to make use of such a macro, so I didn't go for it, but it does > > remove some boiler-plate code. > > > > Let me know what do you think of it ? > > > > Regards, > > Sudeep > > > > -->8 > > diff --git i/include/linux/device/faux.h w/include/linux/device/faux.h > > index 9f43c0e46aa4..8af3eaef281a 100644 > > --- i/include/linux/device/faux.h > > +++ w/include/linux/device/faux.h > > @@ -66,4 +66,30 @@ static inline void faux_device_set_drvdata(struct faux_device *faux_dev, void *d > > dev_set_drvdata(&faux_dev->dev, data); > > } > > > > +#define module_faux_driver(name, tag, init_cond) \ > > +static struct faux_device_ops tag##_ops = { \ > > + .probe = tag##_probe, \ > > + .remove = tag##_remove, \ > > +}; \ > > + \ > > +static struct faux_device *tag##_dev; \ > > + \ > > +static int __init tag##_init(void) \ > > +{ \ > > + if (!(init_cond)) \ > > + return 0; \ > > + tag##_dev = faux_device_create(name, NULL, &tag##_ops); \ > > + if (!tag##_dev) \ > > + return -ENODEV; \ > > + \ > > + return 0; \ > > +} \ > > +module_init(tag##_init); \ > > + \ > > +static void __exit tag##_exit(void) \ > > +{ \ > > + faux_device_destroy(tag##_dev); \ > > +} \ > > +module_exit(tag##_exit); \ > > Yes, I see that some of your changes could be moved to use this, so I > think it is worth it. > > But why can't you use module_driver() here? Ah, that init_cond? And > the device. Hm, why not put the init_cond in the probe callback? That > should make this macro simpler. > I tried to keep the creation of the device itself conditional the way it is today. Deferring the check to probe means the device gets created unconditionally but won't be probed which is fine I guess. I was thinking that device shouldn't show up on the bus if the condition is not met to match the current scenario. I might be overthinking there. > And don't use "tag", that's an odd term here, just copy what > module_driver() does instead please. > Sure, I will not use it. It was just a name that came to my mind. Also creating the macro builds the dependency. Do you prefer to push the changes as is and the macro in one release and make use of the macro later.
On Mon, Mar 17, 2025 at 02:43:21PM +0000, Sudeep Holla wrote: > On Mon, Mar 17, 2025 at 03:30:15PM +0100, Greg Kroah-Hartman wrote: > > On Mon, Mar 17, 2025 at 02:22:45PM +0000, Sudeep Holla wrote: > > > On Mon, Mar 17, 2025 at 02:04:27PM +0100, Greg Kroah-Hartman wrote: > > > > On Mon, Mar 17, 2025 at 10:13:14AM +0000, Sudeep Holla wrote: > > > > > +MODULE_ALIAS("faux:smccc_trng"); > > > > > > > > Why do you need a branch new alias you just made up? Please don't add > > > > that for these types of devices, that's not going to work at all (just > > > > like the platform alias really doesn't work well. > > > > > > > > > > Sure I will drop all of those alias. One question I have if the idea of > > > creating a macro for this is good or bad ? I need this initial condition > > > flag to make use of such a macro, so I didn't go for it, but it does > > > remove some boiler-plate code. > > > > > > Let me know what do you think of it ? > > > > > > Regards, > > > Sudeep > > > > > > -->8 > > > diff --git i/include/linux/device/faux.h w/include/linux/device/faux.h > > > index 9f43c0e46aa4..8af3eaef281a 100644 > > > --- i/include/linux/device/faux.h > > > +++ w/include/linux/device/faux.h > > > @@ -66,4 +66,30 @@ static inline void faux_device_set_drvdata(struct faux_device *faux_dev, void *d > > > dev_set_drvdata(&faux_dev->dev, data); > > > } > > > > > > +#define module_faux_driver(name, tag, init_cond) \ > > > +static struct faux_device_ops tag##_ops = { \ > > > + .probe = tag##_probe, \ > > > + .remove = tag##_remove, \ > > > +}; \ > > > + \ > > > +static struct faux_device *tag##_dev; \ > > > + \ > > > +static int __init tag##_init(void) \ > > > +{ \ > > > + if (!(init_cond)) \ > > > + return 0; \ > > > + tag##_dev = faux_device_create(name, NULL, &tag##_ops); \ > > > + if (!tag##_dev) \ > > > + return -ENODEV; \ > > > + \ > > > + return 0; \ > > > +} \ > > > +module_init(tag##_init); \ > > > + \ > > > +static void __exit tag##_exit(void) \ > > > +{ \ > > > + faux_device_destroy(tag##_dev); \ > > > +} \ > > > +module_exit(tag##_exit); \ > > > > Yes, I see that some of your changes could be moved to use this, so I > > think it is worth it. > > > > But why can't you use module_driver() here? Ah, that init_cond? And > > the device. Hm, why not put the init_cond in the probe callback? That > > should make this macro simpler. > > > > I tried to keep the creation of the device itself conditional the way > it is today. Deferring the check to probe means the device gets created > unconditionally but won't be probed which is fine I guess. I was thinking > that device shouldn't show up on the bus if the condition is not met to > match the current scenario. I might be overthinking there. It will not show up anywhere if the probe call fails. > > And don't use "tag", that's an odd term here, just copy what > > module_driver() does instead please. > > > > Sure, I will not use it. It was just a name that came to my mind. > > Also creating the macro builds the dependency. Do you prefer to push the > changes as is and the macro in one release and make use of the macro later. Let's see a series that adds the macro and uses it and we can figure it out from there. If the macro is sane, I can just take that now for 6.15-rc1 and then you can send the others to the different subsystems after that shows up. thanks, greg k-h
On Mon, Mar 17, 2025 at 05:46:46PM +0100, Greg Kroah-Hartman wrote: > On Mon, Mar 17, 2025 at 02:43:21PM +0000, Sudeep Holla wrote: > > On Mon, Mar 17, 2025 at 03:30:15PM +0100, Greg Kroah-Hartman wrote: > > > On Mon, Mar 17, 2025 at 02:22:45PM +0000, Sudeep Holla wrote: > > > > On Mon, Mar 17, 2025 at 02:04:27PM +0100, Greg Kroah-Hartman wrote: > > > > > On Mon, Mar 17, 2025 at 10:13:14AM +0000, Sudeep Holla wrote: > > > > > > +MODULE_ALIAS("faux:smccc_trng"); > > > > > > > > > > Why do you need a branch new alias you just made up? Please don't add > > > > > that for these types of devices, that's not going to work at all (just > > > > > like the platform alias really doesn't work well. > > > > > > > > > > > > > Sure I will drop all of those alias. One question I have if the idea of > > > > creating a macro for this is good or bad ? I need this initial condition > > > > flag to make use of such a macro, so I didn't go for it, but it does > > > > remove some boiler-plate code. > > > > > > > > Let me know what do you think of it ? > > > > > > > > Regards, > > > > Sudeep > > > > > > > > -->8 > > > > diff --git i/include/linux/device/faux.h w/include/linux/device/faux.h > > > > index 9f43c0e46aa4..8af3eaef281a 100644 > > > > --- i/include/linux/device/faux.h > > > > +++ w/include/linux/device/faux.h > > > > @@ -66,4 +66,30 @@ static inline void faux_device_set_drvdata(struct faux_device *faux_dev, void *d > > > > dev_set_drvdata(&faux_dev->dev, data); > > > > } > > > > > > > > +#define module_faux_driver(name, tag, init_cond) \ > > > > +static struct faux_device_ops tag##_ops = { \ > > > > + .probe = tag##_probe, \ > > > > + .remove = tag##_remove, \ > > > > +}; \ > > > > + \ > > > > +static struct faux_device *tag##_dev; \ > > > > + \ > > > > +static int __init tag##_init(void) \ > > > > +{ \ > > > > + if (!(init_cond)) \ > > > > + return 0; \ > > > > + tag##_dev = faux_device_create(name, NULL, &tag##_ops); \ > > > > + if (!tag##_dev) \ > > > > + return -ENODEV; \ > > > > + \ > > > > + return 0; \ > > > > +} \ > > > > +module_init(tag##_init); \ > > > > + \ > > > > +static void __exit tag##_exit(void) \ > > > > +{ \ > > > > + faux_device_destroy(tag##_dev); \ > > > > +} \ > > > > +module_exit(tag##_exit); \ > > > > > > Yes, I see that some of your changes could be moved to use this, so I > > > think it is worth it. > > > > > > But why can't you use module_driver() here? Ah, that init_cond? And > > > the device. Hm, why not put the init_cond in the probe callback? That > > > should make this macro simpler. > > > > > > > I tried to keep the creation of the device itself conditional the way > > it is today. Deferring the check to probe means the device gets created > > unconditionally but won't be probed which is fine I guess. I was thinking > > that device shouldn't show up on the bus if the condition is not met to > > match the current scenario. I might be overthinking there. > > It will not show up anywhere if the probe call fails. Ah, nice. I somehow didn't realise that. Thanks for that info. > > > > And don't use "tag", that's an odd term here, just copy what > > > module_driver() does instead please. > > > > > > > Sure, I will not use it. It was just a name that came to my mind. > > > > Also creating the macro builds the dependency. Do you prefer to push the > > changes as is and the macro in one release and make use of the macro later. > > Let's see a series that adds the macro and uses it and we can figure it > out from there. If the macro is sane, I can just take that now for > 6.15-rc1 and then you can send the others to the different subsystems > after that shows up. > Sure, thanks again.
diff --git a/drivers/char/hw_random/arm_smccc_trng.c b/drivers/char/hw_random/arm_smccc_trng.c index dcb8e7f37f25c6b39f76050369b9f324b7fb2e33..2ceab17f6360baaee999a23f3d7370b7b5b7d246 100644 --- a/drivers/char/hw_random/arm_smccc_trng.c +++ b/drivers/char/hw_random/arm_smccc_trng.c @@ -16,9 +16,11 @@ #include <linux/device.h> #include <linux/hw_random.h> #include <linux/module.h> -#include <linux/platform_device.h> +#include <linux/device/faux.h> #include <linux/arm-smccc.h> +#include <asm/archrandom.h> + #ifdef CONFIG_ARM64 #define ARM_SMCCC_TRNG_RND ARM_SMCCC_TRNG_RND64 #define MAX_BITS_PER_CALL (3 * 64UL) @@ -33,6 +35,8 @@ #define SMCCC_RET_TRNG_INVALID_PARAMETER -2 #define SMCCC_RET_TRNG_NO_ENTROPY -3 +bool __ro_after_init smccc_trng_available; + static int copy_from_registers(char *buf, struct arm_smccc_res *res, size_t bytes) { @@ -94,29 +98,43 @@ static int smccc_trng_read(struct hwrng *rng, void *data, size_t max, bool wait) return copied; } -static int smccc_trng_probe(struct platform_device *pdev) +static int smccc_trng_probe(struct faux_device *fdev) { struct hwrng *trng; - trng = devm_kzalloc(&pdev->dev, sizeof(*trng), GFP_KERNEL); + trng = devm_kzalloc(&fdev->dev, sizeof(*trng), GFP_KERNEL); if (!trng) return -ENOMEM; trng->name = "smccc_trng"; trng->read = smccc_trng_read; - return devm_hwrng_register(&pdev->dev, trng); + return devm_hwrng_register(&fdev->dev, trng); } -static struct platform_driver smccc_trng_driver = { - .driver = { - .name = "smccc_trng", - }, - .probe = smccc_trng_probe, +static struct faux_device_ops smccc_trng_ops = { + .probe = smccc_trng_probe, }; -module_platform_driver(smccc_trng_driver); -MODULE_ALIAS("platform:smccc_trng"); +static int __init smccc_trng_init(void) +{ + struct faux_device *fdev; + + smccc_trng_available = smccc_probe_trng(); + if (!smccc_trng_available) + return 0; + + fdev = faux_device_create("smccc_trng", NULL, &smccc_trng_ops); + if (!fdev) { + pr_err("smccc_trng: could not create the device\n"); + return -ENODEV; + } + + return 0; +} +device_initcall(smccc_trng_init); + +MODULE_ALIAS("faux:smccc_trng"); MODULE_AUTHOR("Andre Przywara"); MODULE_DESCRIPTION("Arm SMCCC TRNG firmware interface support"); MODULE_LICENSE("GPL"); diff --git a/drivers/firmware/smccc/smccc.c b/drivers/firmware/smccc/smccc.c index a74600d9f2d72a5aa0096004f53088c255927a43..0fcd175a53eeaa957d06071b3b26f4c3a3c7116e 100644 --- a/drivers/firmware/smccc/smccc.c +++ b/drivers/firmware/smccc/smccc.c @@ -9,13 +9,10 @@ #include <linux/init.h> #include <linux/arm-smccc.h> #include <linux/kernel.h> -#include <linux/platform_device.h> -#include <asm/archrandom.h> static u32 smccc_version = ARM_SMCCC_VERSION_1_0; static enum arm_smccc_conduit smccc_conduit = SMCCC_CONDUIT_NONE; -bool __ro_after_init smccc_trng_available = false; s32 __ro_after_init smccc_soc_id_version = SMCCC_RET_NOT_SUPPORTED; s32 __ro_after_init smccc_soc_id_revision = SMCCC_RET_NOT_SUPPORTED; @@ -26,8 +23,6 @@ void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit) smccc_version = version; smccc_conduit = conduit; - smccc_trng_available = smccc_probe_trng(); - if ((smccc_version >= ARM_SMCCC_VERSION_1_2) && (smccc_conduit != SMCCC_CONDUIT_NONE)) { arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_FEATURES_FUNC_ID, @@ -66,19 +61,3 @@ s32 arm_smccc_get_soc_id_revision(void) return smccc_soc_id_revision; } EXPORT_SYMBOL_GPL(arm_smccc_get_soc_id_revision); - -static int __init smccc_devices_init(void) -{ - struct platform_device *pdev; - - if (smccc_trng_available) { - pdev = platform_device_register_simple("smccc_trng", -1, - NULL, 0); - if (IS_ERR(pdev)) - pr_err("smccc_trng: could not register device: %ld\n", - PTR_ERR(pdev)); - } - - return 0; -} -device_initcall(smccc_devices_init);
The Arm SMCCC based true random number generator driver does not require the creation of a platform device/driver. Originally, this approach was chosen for simplicity when the driver was first implemented. With the introduction of the lightweight faux device interface, we now have a more appropriate alternative. Migrate the driver to utilize the faux bus, given that the platform device it previously created was not a real one anyway. This will simplify the code, reducing its footprint while maintaining functionality. Cc: Andre Przywara <andre.przywara@arm.com> Cc: Herbert Xu <herbert@gondor.apana.org.au> Cc: Jeff Johnson <jeff.johnson@oss.qualcomm.com> Cc: linux-crypto@vger.kernel.org Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/char/hw_random/arm_smccc_trng.c | 40 ++++++++++++++++++++++++--------- drivers/firmware/smccc/smccc.c | 21 ----------------- 2 files changed, 29 insertions(+), 32 deletions(-)