Re: [PATCH v9 2/7] firmware: hwrng: arm_smccc_trng: Register as an SMCCC device

[Aneesh Kumar K . V]

Jason Gunthorpe <jgg@xxxxxxxxxx> writes:

>> [ ... 58 lines skipped ... ]
>> @@ -94,29 +96,37 @@ 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 arm_smccc_device *sdev)
>>  {
>>  	struct hwrng *trng;
>>  
>> -	trng = devm_kzalloc(&pdev->dev, sizeof(*trng), GFP_KERNEL);
>> +	/* validate the minimum version requirement */
>> +	if (!smccc_probe_trng())
>> +		return -ENODEV;
>
> It feels like slightly poor practice to do this.. It is doing three
> things:
>
> 1) ARM32 disables this entirely for some reason, shouldn't the bus do
>    it? Maybe it already does?
>

Why? The bus only checks whether the firmware function is supported and
creates the device if it is. Further validation should be the driver's
responsibility, shouldn't it?

>
> 2) Checks the API exists and checks but the bus already did this.
>

This checks that the firmware supports the minimum ABI version required
by the driver.

>
> 3) Checks the version number
>
> Maybe the bus should capture the version output and pass it in as an
> argument to probe so the driver can do the min version check directly?
>

One of the earlier discussions suggested that the bus should only check
whether the function ID is supported, rather than checking for an
expected version or anything similar. This keeps the bus code generic.

	arm_smccc_1_1_invoke(smccc_dev->func_id, &res);
	ret = res.a0;

	if (ret == SMCCC_RET_NOT_SUPPORTED)
		return false;

The other alternative discussed was a device-specific callback that
would perform additional validation and create the device only when
those conditions were met. It was dropped in favor of the simpler bus
code above.

>
> It is very minor anyhow, it looks OK
>
> Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
>
> -- 
> Jason

-aneesh