Received-SPF: pass (google.com: domain of linux-crypto+bounces-4076-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 9 May 2024 20:57:29 +0200
From: Marek =?UTF-8?B?QmVow7pu?= <kabel@kernel.org>
To: Andy Shevchenko <andy@kernel.org>
Cc: Gregory CLEMENT <gregory.clement@bootlin.com>, Arnd Bergmann
 <arnd@arndb.de>, soc@kernel.org, arm@kernel.org, Hans de Goede
 <hdegoede@redhat.com>, Ilpo =?UTF-8?B?SsOkcnZpbmVu?=
 <ilpo.jarvinen@linux.intel.com>, Greg Kroah-Hartman
 <gregkh@linuxfoundation.org>, linux-crypto@vger.kernel.org, Dan Carpenter
 <dan.carpenter@linaro.org>
Subject: Re: [PATCH v9 7/9] platform: cznic: turris-omnia-mcu: Add support
 for digital message signing via debugfs
Message-ID: <20240509205729.09728cbb@thinkpad>
In-Reply-To: <Zjti-FkUCAQzMmrQ@smile.fi.intel.com>
References: <20240508103118.23345-1-kabel@kernel.org>
	<20240508103118.23345-8-kabel@kernel.org>
	<Zjti-FkUCAQzMmrQ@smile.fi.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Wed, 8 May 2024 14:33:12 +0300
Andy Shevchenko <andy@kernel.org> wrote:

> On Wed, May 08, 2024 at 12:31:16PM +0200, Marek Beh=C3=BAn 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 =3D dev_id;
> > +	int err;
> > +
> > +	err =3D omnia_cmd_read(mcu->client, OMNIA_CMD_CRYPTO_COLLECT_SIGNATUR=
E,
> > +			     reply, sizeof(reply));
> > +	if (!err && reply[0] !=3D OMNIA_MCU_CRYPTO_SIGNATURE_LEN)
> > +		err =3D -EIO;
> > +
> > +	guard(mutex)(&mcu->sign_lock);
> > +
> > +	if (mcu->sign_state =3D=3D SIGN_STATE_REQUESTED) {
> > +		mcu->sign_err =3D err;
> > +		if (!err)
> > +			memcpy(mcu->signature, &reply[1],
> > +			       OMNIA_MCU_CRYPTO_SIGNATURE_LEN); =20
>=20
> > +		mcu->sign_state =3D SIGN_STATE_COLLECTED; =20
>=20
> Even for an error case?

Yes, the pair (errno, signature) is collected.

> > +		complete(&mcu->msg_signed_completion);
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +} =20
>=20
> ...
>=20
> > +	scoped_guard(mutex, &mcu->sign_lock)
> > +		if (mcu->sign_state !=3D SIGN_STATE_REQUESTED &&
> > +		    mcu->sign_state !=3D SIGN_STATE_COLLECTED)
> > +			return -ENODATA; =20
>=20
> {}
>=20
> 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.

> ...
>=20
> > +#define OMNIA_MCU_CRYPTO_PUBLIC_KEY_LEN	33 =20
>=20
> 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