On Wed, 8 May 2024 14:33:12 +0300
Andy Shevchenko <andy@xxxxxxxxxx> wrote:
> On Wed, May 08, 2024 at 12:31:16PM +0200, Marek Behún wrote:
...
> > +static irqreturn_t omnia_msg_signed_irq_handler(int irq, void *dev_id)
> > +{
> > + u8 reply[1 + OMNIA_MCU_CRYPTO_SIGNATURE_LEN];
> > + struct omnia_mcu *mcu = dev_id;
> > + int err;
> > +
> > + err = omnia_cmd_read(mcu->client, OMNIA_CMD_CRYPTO_COLLECT_SIGNATURE,
> > + reply, sizeof(reply));
> > + if (!err && reply[0] != OMNIA_MCU_CRYPTO_SIGNATURE_LEN)
> > + err = -EIO;
> > +
> > + guard(mutex)(&mcu->sign_lock);
> > +
> > + if (mcu->sign_state == SIGN_STATE_REQUESTED) {
> > + mcu->sign_err = err;
> > + if (!err)
> > + memcpy(mcu->signature, &reply[1],
> > + OMNIA_MCU_CRYPTO_SIGNATURE_LEN);
>
> > + mcu->sign_state = SIGN_STATE_COLLECTED;
>
> Even for an error case?
Yes, the pair (errno, signature) is collected.
> > + complete(&mcu->msg_signed_completion);
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
>
> ...
>
> > + scoped_guard(mutex, &mcu->sign_lock)
> > + if (mcu->sign_state != SIGN_STATE_REQUESTED &&
> > + mcu->sign_state != SIGN_STATE_COLLECTED)
> > + return -ENODATA;
>
> {}
>
> Don't you want interruptible mutex? In such case you might need to return
> -ERESTARTSYS. OTOH, this is debugfs, we don't much care.
Indeed I shall use
scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &mcu->sign_lock) {
...
}
And -ERESTARTSYS instead of -EINTR also for the subsequent
wait_for_completion_interruptible(), and also in trng from patch 6/9.
> ...
>
> > +#define OMNIA_MCU_CRYPTO_PUBLIC_KEY_LEN 33
>
> 33? Hmm... does it mean (32 + 1)?
Rather (1 + 32), the first byte is 0x02 or 0x03, determining whether
the y coordinate of the public key elliptic curve point is positive or
negative, and the rest 32 bytes are the x coordinate.
Marek
