| Message ID | 1449388004.2096.12.camel@hbabu-laptop (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
Haren Myneni <haren@linux.vnet.ibm.com> writes: > NX842 coprocessor sets bit 3 if queue is overflow. It is just for > information to the user. So the driver prints this informative message > and ignores it. What queue, and what happens when the queue overflows? It seems like *something* would need to be done, somewhere, by someone? I realise that as a piece of IBM hardware this is probably an incredibly optimistic question, but is this behaviour documented publically anywhere? (As a distant second best, is it documented internally anywhere that I can read?) > --- a/drivers/crypto/nx/nx-842-powernv.c > +++ b/drivers/crypto/nx/nx-842-powernv.c > @@ -442,6 +442,15 @@ static int nx842_powernv_function(const unsigned char *in, unsigned int inlen, > (unsigned int)ccw, > (unsigned int)be32_to_cpu(crb->ccw)); > > + /* > + * NX842 coprocessor uses 3rd bit to report queue overflow which is > + * not an error, just for information to user. So, ignore this bit. > + */ > + if (ret & ICSWX_BIT3) { > + pr_info_ratelimited("842 coprocessor queue overflow\n"); It doesn't look like this is done anywhere else in the file, but should this be prefixed with something? Something like "nx-842: Coprocessor queue overflow"? Regards, Daniel > + ret &= ~ICSWX_BIT3; > + } > + > switch (ret) { > case ICSWX_INITIATED: > ret = wait_for_csb(wmem, csb); > > > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev
On Sun, Dec 6, 2015 at 2:46 AM, Haren Myneni <haren@linux.vnet.ibm.com> wrote: > > NX842 coprocessor sets bit 3 if queue is overflow. It is just for > information to the user. So the driver prints this informative message > and ignores it. > > Signed-off-by: Haren Myneni <haren@us.ibm.com> > > diff --git a/arch/powerpc/include/asm/icswx.h b/arch/powerpc/include/asm/icswx.h > index 9f8402b..d1a2a2d 100644 > --- a/arch/powerpc/include/asm/icswx.h > +++ b/arch/powerpc/include/asm/icswx.h > @@ -164,6 +164,7 @@ struct coprocessor_request_block { > #define ICSWX_INITIATED (0x8) > #define ICSWX_BUSY (0x4) > #define ICSWX_REJECTED (0x2) > +#define ICSWX_BIT3 (0x1) /* undefined or set from XERSO. */ Since this isn't defined by the icswx rfc workbook, it probably shouldn't go here, it would make more sense to put it into nx-842.h and call it something like "ICSWX_NX_QUEUE_OVERFLOW" or similar NX-specific meaningful name. > > static inline int icswx(__be32 ccw, struct coprocessor_request_block *crb) > { > diff --git a/drivers/crypto/nx/nx-842-powernv.c b/drivers/crypto/nx/nx-842-powernv.c > index 9ef51fa..321b8e8 100644 > --- a/drivers/crypto/nx/nx-842-powernv.c > +++ b/drivers/crypto/nx/nx-842-powernv.c > @@ -442,6 +442,15 @@ static int nx842_powernv_function(const unsigned char *in, unsigned int inlen, > (unsigned int)ccw, > (unsigned int)be32_to_cpu(crb->ccw)); > > + /* > + * NX842 coprocessor uses 3rd bit to report queue overflow which is > + * not an error, just for information to user. So, ignore this bit. > + */ a meaningfully named bit define means you don't need to explain it with a comment :-) However, I suggest that you do explain *why* a queue overflow isn't an error - either here or (probably better) at the #define of the bit - because that isn't obvious. > + if (ret & ICSWX_BIT3) { > + pr_info_ratelimited("842 coprocessor queue overflow\n"); if it's not an error, should this be pr_debug_ratelimited instead? What is an end user expected to do if they see this msg in the log? > + ret &= ~ICSWX_BIT3; > + } > + > switch (ret) { > case ICSWX_INITIATED: > ret = wait_for_csb(wmem, csb); > > -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Dec 6, 2015 at 5:57 PM, Daniel Axtens <dja@axtens.net> wrote: > Haren Myneni <haren@linux.vnet.ibm.com> writes: > >> NX842 coprocessor sets bit 3 if queue is overflow. It is just for >> information to the user. So the driver prints this informative message >> and ignores it. > > What queue, and what happens when the queue overflows? It seems like > *something* would need to be done, somewhere, by someone? > > I realise that as a piece of IBM hardware this is probably an incredibly > optimistic question, but is this behaviour documented publically anywhere? > (As a distant second best, is it documented internally anywhere that I > can read?) When I worked there, it unfortunately wasn't public and there was no future plan to make it public, but things might have changed since I left. Maybe it will be included in future openpower documentation...? > >> --- a/drivers/crypto/nx/nx-842-powernv.c >> +++ b/drivers/crypto/nx/nx-842-powernv.c >> @@ -442,6 +442,15 @@ static int nx842_powernv_function(const unsigned char *in, unsigned int inlen, >> (unsigned int)ccw, >> (unsigned int)be32_to_cpu(crb->ccw)); >> >> + /* >> + * NX842 coprocessor uses 3rd bit to report queue overflow which is >> + * not an error, just for information to user. So, ignore this bit. >> + */ >> + if (ret & ICSWX_BIT3) { >> + pr_info_ratelimited("842 coprocessor queue overflow\n"); > It doesn't look like this is done anywhere else in the file, but should > this be prefixed with something? Something like "nx-842: Coprocessor > queue overflow"? it defines pr_fmt at the top of the file so it will be prefixed with the module name. > > Regards, > Daniel > >> + ret &= ~ICSWX_BIT3; >> + } >> + >> switch (ret) { >> case ICSWX_INITIATED: >> ret = wait_for_csb(wmem, csb); >> >> >> _______________________________________________ >> Linuxppc-dev mailing list >> Linuxppc-dev@lists.ozlabs.org >> https://lists.ozlabs.org/listinfo/linuxppc-dev -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/07/2015 11:34 AM, Dan Streetman wrote: > On Sun, Dec 6, 2015 at 2:46 AM, Haren Myneni <haren@linux.vnet.ibm.com> wrote: >> >> NX842 coprocessor sets bit 3 if queue is overflow. It is just for >> information to the user. So the driver prints this informative message >> and ignores it. >> >> Signed-off-by: Haren Myneni <haren@us.ibm.com> >> >> diff --git a/arch/powerpc/include/asm/icswx.h b/arch/powerpc/include/asm/icswx.h >> index 9f8402b..d1a2a2d 100644 >> --- a/arch/powerpc/include/asm/icswx.h >> +++ b/arch/powerpc/include/asm/icswx.h >> @@ -164,6 +164,7 @@ struct coprocessor_request_block { >> #define ICSWX_INITIATED (0x8) >> #define ICSWX_BUSY (0x4) >> #define ICSWX_REJECTED (0x2) >> +#define ICSWX_BIT3 (0x1) /* undefined or set from XERSO. */ > > Since this isn't defined by the icswx rfc workbook, it probably > shouldn't go here, it would make more sense to put it into nx-842.h > and call it something like "ICSWX_NX_QUEUE_OVERFLOW" or similar > NX-specific meaningful name. This bit is defined in icswx RFC. Hence I think we should define this in icswx.h. "Bit 3 of CR0 is undefined or set from XERSO." Please ignore this patch. Talking to HW team, whenever gets floating point overflow from FPU, XER[S0] will be set and it stays until other FPU operation is executed. It is typical behaviour on powerpc. ixswx RFC says coprocessor can set this XER[S0] to bit 3 and NX is doing this. I think it should have ignored this bit. "An implementation is permitted to set bit 3 of CR0 from XERSO." So,the issue is not queue overflow problem, but NX is copying XER[S0] which is no use and nothing to do with compression. We need to ignore this bit since it can be set with other valuable return status. I will repost new patch with the proper description. Thanks Haren > >> >> static inline int icswx(__be32 ccw, struct coprocessor_request_block *crb) >> { >> diff --git a/drivers/crypto/nx/nx-842-powernv.c b/drivers/crypto/nx/nx-842-powernv.c >> index 9ef51fa..321b8e8 100644 >> --- a/drivers/crypto/nx/nx-842-powernv.c >> +++ b/drivers/crypto/nx/nx-842-powernv.c >> @@ -442,6 +442,15 @@ static int nx842_powernv_function(const unsigned char *in, unsigned int inlen, >> (unsigned int)ccw, >> (unsigned int)be32_to_cpu(crb->ccw)); >> >> + /* >> + * NX842 coprocessor uses 3rd bit to report queue overflow which is >> + * not an error, just for information to user. So, ignore this bit. >> + */ > > a meaningfully named bit define means you don't need to explain it > with a comment :-) > > However, I suggest that you do explain *why* a queue overflow isn't an > error - either here or (probably better) at the #define of the bit - > because that isn't obvious. > >> + if (ret & ICSWX_BIT3) { >> + pr_info_ratelimited("842 coprocessor queue overflow\n"); > > if it's not an error, should this be pr_debug_ratelimited instead? > What is an end user expected to do if they see this msg in the log? > >> + ret &= ~ICSWX_BIT3; >> + } >> + >> switch (ret) { >> case ICSWX_INITIATED: >> ret = wait_for_csb(wmem, csb); >> >> > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev > -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/powerpc/include/asm/icswx.h b/arch/powerpc/include/asm/icswx.h index 9f8402b..d1a2a2d 100644 --- a/arch/powerpc/include/asm/icswx.h +++ b/arch/powerpc/include/asm/icswx.h @@ -164,6 +164,7 @@ struct coprocessor_request_block { #define ICSWX_INITIATED (0x8) #define ICSWX_BUSY (0x4) #define ICSWX_REJECTED (0x2) +#define ICSWX_BIT3 (0x1) /* undefined or set from XERSO. */ static inline int icswx(__be32 ccw, struct coprocessor_request_block *crb) { diff --git a/drivers/crypto/nx/nx-842-powernv.c b/drivers/crypto/nx/nx-842-powernv.c index 9ef51fa..321b8e8 100644 --- a/drivers/crypto/nx/nx-842-powernv.c +++ b/drivers/crypto/nx/nx-842-powernv.c @@ -442,6 +442,15 @@ static int nx842_powernv_function(const unsigned char *in, unsigned int inlen, (unsigned int)ccw, (unsigned int)be32_to_cpu(crb->ccw)); + /* + * NX842 coprocessor uses 3rd bit to report queue overflow which is + * not an error, just for information to user. So, ignore this bit. + */ + if (ret & ICSWX_BIT3) { + pr_info_ratelimited("842 coprocessor queue overflow\n"); + ret &= ~ICSWX_BIT3; + } + switch (ret) { case ICSWX_INITIATED: ret = wait_for_csb(wmem, csb);
NX842 coprocessor sets bit 3 if queue is overflow. It is just for information to the user. So the driver prints this informative message and ignores it. Signed-off-by: Haren Myneni <haren@us.ibm.com> -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html