Re: [PATCH 2/9] hwrng: arm-smccc-trng - transition to the faux device interface

[Greg Kroah-Hartman]

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