Re: [PATCH] Silicon/Atmel: add support for AtSha204a RNG

[Leif Lindholm <leif.lindholm@linaro.org>]

On Mon, Feb 19, 2018 at 06:19:53PM +0000, Ard Biesheuvel wrote:
> >> +/**
> >> +  Produces and returns an RNG value using either the default or specified RNG
> >> +  algorithm.
> >> +
> >> +  @param[in]  This                A pointer to the EFI_RNG_PROTOCOL instance.
> >> +  @param[in]  Algorithm           A pointer to the EFI_RNG_ALGORITHM that
> >> +                                  identifies the RNG algorithm to use. May be
> >> +                                  NULL in which case the function will use its
> >> +                                  default RNG algorithm.
> >> +  @param[in]  ValueLength         The length in bytes of the memory buffer
> >> +                                  pointed to by RNGValue. The driver shall
> >> +                                  return exactly this numbers of bytes.
> >> +  @param[out] Value               A caller-allocated memory buffer filled by the
> >> +                                  driver with the resulting RNG value.
> >> +
> >> +  @retval EFI_SUCCESS             The RNG value was returned successfully.
> >> +  @retval EFI_UNSUPPORTED         The algorithm specified by RNGAlgorithm is not
> >> +                                  supported by this driver.
> >> +  @retval EFI_DEVICE_ERROR        An RNG value could not be retrieved due to a
> >> +                                  hardware or firmware error.
> >> +  @retval EFI_NOT_READY           There is not enough random data available to
> >> +                                  satisfy the length requested by
> >> +                                  RNGValueLength.
> >> +  @retval EFI_INVALID_PARAMETER   RNGValue is NULL or RNGValueLength is zero.
> >> +
> >> +**/
> >> +STATIC
> >> +EFI_STATUS
> >> +EFIAPI
> >> +AtSha240aGetRNG (
> >> +  IN EFI_RNG_PROTOCOL   *This,
> >> +  IN EFI_RNG_ALGORITHM  *Algorithm OPTIONAL,
> >> +  IN UINTN              ValueLength,
> >> +  OUT UINT8             *Value
> >> +)
> >> +{
> >> +  EFI_STATUS                  Status;
> >> +  ATSHA204A_DEV               *AtSha204a;
> >> +  ATSHA204A_I2C_RNG_COMMAND   Cmd;
> >> +  ATSHA204A_I2C_RNG_RESULT    Res;
> >> +  I2C_RNG_REQUEST             Req;
> >> +  I2C_RNG_REQUEST             Resp;
> >
> > Since I just gave Marcin a hard time over non-approved abbreviations,
> > I should probably flag these too... Would you mind terribly turning
> > them into Command, Result, Request, Response?
> >
> 
> No that is fine.
> 
> >> +  UINTN                       Retries;
> >> +
> >> +  if (Algorithm != NULL && !CompareGuid (Algorithm, &gEfiRngAlgorithmRaw)) {
> >> +    return EFI_UNSUPPORTED;
> >> +  }
> >> +
> >> +  AtSha204a = ATSHA204A_DEV_FROM_THIS (This);
> >> +
> >> +  Req.OperationCount  = 1;
> >> +  Req.Operation.Flags = 0;
> >> +
> >> +  Cmd.Command   = ATSHA204A_COMMAND;
> >> +  Cmd.Count     = sizeof (Cmd) - 1;
> >> +  Cmd.Opcode    = ATSHA204A_OPCODE_RANDOM;
> >> +  Cmd.Param1    = 0;
> >> +  Cmd.Param2    = 0;
> >> +  Cmd.Crc       = OPCODE_COMMAND_PACKET_CRC;
> >> +
> >> +  Resp.OperationCount           = 1;
> >> +  Resp.Operation.Flags          = I2C_FLAG_READ;
> >> +  Resp.Operation.LengthInBytes  = sizeof (Res);
> >> +  Resp.Operation.Buffer         = (VOID *)&Res;
> >> +
> >> +  Retries = 0;
> >> +  while (ValueLength > 0) {
> >> +    //
> >> +    // The AtSha204a will go back to sleep right in the middle of a transaction
> >> +    // if it does not complete in ~1.3 seconds. So send the wake sequence for
> >> +    // each iteration, which consists of a dummy write to slave address 0x0.
> >> +    //
> >> +    Req.Operation.LengthInBytes = 0;
> >> +    Status = AtSha204a->I2cIo->QueueRequest (AtSha204a->I2cIo, 1, NULL,
> >> +                                 (VOID *)&Req, NULL);
> >> +    DEBUG ((DEBUG_INFO, "%a: wake AtSha204a: I2cIo->QueueRequest() - %r\n",
> >> +      __FUNCTION__, Status));
> >> +
> >> +    gBS->Stall (2500); // wait 2.5 ms for wake to complete
> >> +
> >> +    Req.Operation.LengthInBytes = sizeof (Cmd);
> >> +    Req.Operation.Buffer = (VOID *)&Cmd;
> >> +    Status = AtSha204a->I2cIo->QueueRequest (AtSha204a->I2cIo, 0, NULL,
> >> +                                 (VOID *)&Req, NULL);
> >> +    if (EFI_ERROR (Status)) {
> >> +      if (++Retries <= MAX_RETRIES) {
> >> +        continue;
> >> +      }
> >> +      DEBUG ((DEBUG_ERROR, "%a: I2C request transfer failed, Status == %r\n",
> >> +        __FUNCTION__, Status));
> >> +      return EFI_DEVICE_ERROR;
> >> +    }
> >> +
> >> +    gBS->Stall (50 * 1000); // 50 ms max execution time for RANDOM opcode
> >
> > Is there no way of polling for response complete?
> > 50ms per iteration?? :(  (or, well, 52.5 in total)
> >
> 
> Typical execution time is 11 ms, but it is rare for this routine to
> iterate more than once in the first place, so I tried to keep it
> simple. (The typical use case of a raw RNG is to seed a DRBG, which
> typically takes 32 bytes of seed, which is exactly the output size of
> a single invocation)
> 
> I don't mind changing it, though, if you feel strongly about this.

Well, decreasing the explicit delay within the same order of magnitude
doesn't seem like a very good return on investment. If it was possible
to get rid of the explicit delay, I'd be all for putting some effort
into that.

It's just a bit of a shame with a bandwidth of < 1kb/s.
But if that's the hardware limitation, that's the hardware limitation.

/
    Leif