> -----Original Message----- > From: Pascal Van Leeuwen > Sent: Thursday, October 17, 2019 7:14 PM > To: 'Ben Dooks (Codethink)' <[email protected]>; linux- > [email protected] > Cc: Antoine Tenart <[email protected]>; Herbert Xu > <[email protected]>; David S. Miller <[email protected]>; linux- > [email protected]; [email protected] > Subject: RE: [PATCH] crypto: inside-secure - fix type of buffer in > eip197_write_firmware > > > -----Original Message----- > > From: [email protected] > > <[email protected]> On Behalf > Of Ben > > Dooks (Codethink) > > Sent: Wednesday, October 16, 2019 1:50 PM > > To: [email protected] > > Cc: Ben Dooks (Codethink) <[email protected]>; Antoine Tenart > > <[email protected]>; Herbert Xu <[email protected]>; > > David S. Miller > > <[email protected]>; [email protected]; > > [email protected] > > Subject: [PATCH] crypto: inside-secure - fix type of buffer in > > eip197_write_firmware > > > > In eip197_write_firmware() the firmware buffer is sent using > > writel(be32_to_cpu(),,,) this produces a number of warnings. > > > > Note, should this really be cpu_to_be32() ? > > > No, it should certainly not be cpu_to_be32() since the HW itself is most > definitely little endian, so that would not make sense to me. > > Actually, I don't think either solution would be correct on a big-endian > CPU. But I don't have any big-endian CPU available to test that theory. > > What I believe must happen is that the bytes must *always* be swapped > here, regardless of the endianness of the CPU. And with a little-endian > CPU, be32_to_cpu() coincidentally always does that. > > Basically, what we need here is: read a dword (32 bits) from the memory > subsystem and write it back to the memory subsystem with bytes reversed. > > Does the kernel have any dedicated function for just always swapping? >
After some more thought on the train home: I think the correct construct would be cpu_to_le32(be32_to_cpu(data[i])) This would correctly reflect that the data is read from big-endian memory and subsequently written to little-endian "memory" (aka the EIP). It also fits in nicely with your other changes. Could you work that into a patch v2? Then I would ack it (after testing). > Anyway: NACK on this patch for now due to this. > > > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted > > __be32 > > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted > > __be32 > > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted > > __be32 > > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted > > __be32 > > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted > > __be32 > > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted > > __be32 > > > > Signed-off-by: Ben Dooks <[email protected]> > > --- > > Cc: Antoine Tenart <[email protected]> > > Cc: Herbert Xu <[email protected]> > > Cc: "David S. Miller" <[email protected]> > > Cc: [email protected] > > Cc: [email protected] > > --- > > drivers/crypto/inside-secure/safexcel.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/crypto/inside-secure/safexcel.c > > b/drivers/crypto/inside- > secure/safexcel.c > > index 223d1bfdc7e6..dd33f6dda295 100644 > > --- a/drivers/crypto/inside-secure/safexcel.c > > +++ b/drivers/crypto/inside-secure/safexcel.c > > @@ -298,13 +298,13 @@ static void eip197_init_firmware(struct > > safexcel_crypto_priv > *priv) > > static int eip197_write_firmware(struct safexcel_crypto_priv *priv, > > const struct firmware *fw) > > { > > - const u32 *data = (const u32 *)fw->data; > > + const __be32 *data = (const __be32 *)fw->data; > > int i; > > > > /* Write the firmware */ > > - for (i = 0; i < fw->size / sizeof(u32); i++) > > + for (i = 0; i < fw->size / sizeof(__be32); i++) > > writel(be32_to_cpu(data[i]), > > - priv->base + EIP197_CLASSIFICATION_RAMS + i * > > sizeof(u32)); > > + priv->base + EIP197_CLASSIFICATION_RAMS + i * > > sizeof(__be32)); > > > > /* Exclude final 2 NOPs from size */ > > return i - EIP197_FW_TERMINAL_NOPS; > > -- > > 2.23.0 > > Regards, > Pascal van Leeuwen > Silicon IP Architect, Multi-Protocol Engines @ Verimatrix > www.insidesecure.com Regards, Pascal van Leeuwen Silicon IP Architect, Multi-Protocol Engines @ Verimatrix www.insidesecure.com
