[PATCH] bus: fsl-mc: dpio: fix alter FQ state command

[PATCH] bus: fsl-mc: dpio: fix alter FQ state command

[Horia Geantă]

When checking the response verb, the valid bit should be masked out,
since its value flips depending on what Response Register
(RR0 /RR1) it's been read from.

Fixes: 321eecb06bfb ("bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2")
Signed-off-by: Horia Geantă <horia.geanta@nxp.com>
---
 drivers/staging/fsl-mc/bus/dpio/qbman-portal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/fsl-mc/bus/dpio/qbman-portal.c b/drivers/staging/fsl-mc/bus/dpio/qbman-portal.c
index 2a3ea29d9b43..5d020fb98c66 100644
--- a/drivers/staging/fsl-mc/bus/dpio/qbman-portal.c
+++ b/drivers/staging/fsl-mc/bus/dpio/qbman-portal.c
@@ -963,7 +963,7 @@ int qbman_swp_alt_fq_state(struct qbman_swp *s, u32 fqid,
 	}
 
 	/* Decode the outcome */
-	WARN_ON(r->verb != alt_fq_verb);
+	WARN_ON((r->verb & 0x7f) != alt_fq_verb);
 
 	/* Determine success or failure */
 	if (unlikely(r->rslt != QBMAN_MC_RSLT_OK)) {
-- 
2.12.0.264.gd6db3f216544

Re: [PATCH] bus: fsl-mc: dpio: fix alter FQ state command

[Stuart Yoder]

On Fri, Apr 21, 2017 at 7:00 AM, Horia Geantă <horia.geanta@nxp.com> wrote:
>
> When checking the response verb, the valid bit should be masked out,
> since its value flips depending on what Response Register
> (RR0 /RR1) it's been read from.
>
> Fixes: 321eecb06bfb ("bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2")
> Signed-off-by: Horia Geantă <horia.geanta@nxp.com>
> ---
>  drivers/staging/fsl-mc/bus/dpio/qbman-portal.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/fsl-mc/bus/dpio/qbman-portal.c b/drivers/staging/fsl-mc/bus/dpio/qbman-portal.c
> index 2a3ea29d9b43..5d020fb98c66 100644
> --- a/drivers/staging/fsl-mc/bus/dpio/qbman-portal.c
> +++ b/drivers/staging/fsl-mc/bus/dpio/qbman-portal.c
> @@ -963,7 +963,7 @@ int qbman_swp_alt_fq_state(struct qbman_swp *s, u32 fqid,
>         }
>
>         /* Decode the outcome */
> -       WARN_ON(r->verb != alt_fq_verb);
> +       WARN_ON((r->verb & 0x7f) != alt_fq_verb);

Don't use magic constants like that.  Create a #define for the valid bit.

Thanks,
Stuart

Re: [PATCH] bus: fsl-mc: dpio: fix alter FQ state command

[Horia Geantă]

On 4/21/2017 4:31 PM, Stuart Yoder wrote:
> On Fri, Apr 21, 2017 at 7:00 AM, Horia Geantă <horia.geanta@nxp.com> wrote:
>>
>> When checking the response verb, the valid bit should be masked out,
>> since its value flips depending on what Response Register
>> (RR0 /RR1) it's been read from.
>>
>> Fixes: 321eecb06bfb ("bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2")
>> Signed-off-by: Horia Geantă <horia.geanta@nxp.com>
>> ---
>>  drivers/staging/fsl-mc/bus/dpio/qbman-portal.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/fsl-mc/bus/dpio/qbman-portal.c b/drivers/staging/fsl-mc/bus/dpio/qbman-portal.c
>> index 2a3ea29d9b43..5d020fb98c66 100644
>> --- a/drivers/staging/fsl-mc/bus/dpio/qbman-portal.c
>> +++ b/drivers/staging/fsl-mc/bus/dpio/qbman-portal.c
>> @@ -963,7 +963,7 @@ int qbman_swp_alt_fq_state(struct qbman_swp *s, u32 fqid,
>>         }
>>
>>         /* Decode the outcome */
>> -       WARN_ON(r->verb != alt_fq_verb);
>> +       WARN_ON((r->verb & 0x7f) != alt_fq_verb);
> 
> Don't use magic constants like that.  Create a #define for the valid bit.
> 
Just looking at the surrounding code, this seems the preferred way to
mask the valid bit.

Though there are two defines that could be used in this case:
#define QB_VALID_BIT ((u32)0x80)
#define QBMAN_RESULT_MASK      0x7f

Let me know whether keeping code similar would be preferred or not.

Thanks,
Horia

Re: [PATCH] bus: fsl-mc: dpio: fix alter FQ state command

[Dan Carpenter]

On Fri, Apr 21, 2017 at 01:50:24PM +0000, Horia Geantă wrote:
> Let me know whether keeping code similar would be preferred or not.
> 

Keeping the code similar to the surrounding is never a valid argument
but especially not in staging/.  If only 1 line is nice in the whole
file, at least that's better than nothing.

regards,
dan carpenter

[PATCH v2] bus: fsl-mc: dpio: fix alter FQ state command

[Horia Geantă]

When checking the response verb, the valid bit should be masked out,
since its value flips depending on what Response Register
(RR0 /RR1) it's been read from.

Fixes: 321eecb06bfb ("bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2")
Signed-off-by: Horia Geantă <horia.geanta@nxp.com>
---
v2: use QBMAN_RESULT_MASK instead of hard-coded mask

 drivers/staging/fsl-mc/bus/dpio/qbman-portal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/fsl-mc/bus/dpio/qbman-portal.c b/drivers/staging/fsl-mc/bus/dpio/qbman-portal.c
index 2a3ea29d9b43..7988612aaecf 100644
--- a/drivers/staging/fsl-mc/bus/dpio/qbman-portal.c
+++ b/drivers/staging/fsl-mc/bus/dpio/qbman-portal.c
@@ -963,7 +963,7 @@ int qbman_swp_alt_fq_state(struct qbman_swp *s, u32 fqid,
 	}
 
 	/* Decode the outcome */
-	WARN_ON(r->verb != alt_fq_verb);
+	WARN_ON((r->verb & QBMAN_RESULT_MASK) != alt_fq_verb);
 
 	/* Determine success or failure */
 	if (unlikely(r->rslt != QBMAN_MC_RSLT_OK)) {
-- 
2.12.0.264.gd6db3f216544

Re: [PATCH v2] bus: fsl-mc: dpio: fix alter FQ state command

[Greg KH]

On Sat, Apr 22, 2017 at 09:44:49AM +0300, Horia Geantă wrote:
> When checking the response verb, the valid bit should be masked out,
> since its value flips depending on what Response Register
> (RR0 /RR1) it's been read from.
> 
> Fixes: 321eecb06bfb ("bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2")
> Signed-off-by: Horia Geantă <horia.geanta@nxp.com>
> ---
> v2: use QBMAN_RESULT_MASK instead of hard-coded mask

Why are you not using 'staging' in your subject line?

thanks,

greg k-h

Re: [PATCH v2] bus: fsl-mc: dpio: fix alter FQ state command

[Horia Geantă]

On 4/28/2017 1:32 PM, Greg KH wrote:
> On Sat, Apr 22, 2017 at 09:44:49AM +0300, Horia Geantă wrote:
>> When checking the response verb, the valid bit should be masked out,
>> since its value flips depending on what Response Register
>> (RR0 /RR1) it's been read from.
>>
>> Fixes: 321eecb06bfb ("bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2")
>> Signed-off-by: Horia Geantă <horia.geanta@nxp.com>
>> ---
>> v2: use QBMAN_RESULT_MASK instead of hard-coded mask
> 
> Why are you not using 'staging' in your subject line?
> 
I was not aware this is required and followed the statistics:

git log --oneline drivers/staging/fsl-mc/bus/dpio/
141a10aead9c staging: fsl-mc/dpio: Fix early writing of valid bit
993fec7e11ca bus: fsl-mc: dpio: add the DPAA2 DPIO object driver
780b626323d7 bus: fsl-mc: dpio: add the DPAA2 DPIO service interface
321eecb06bfb bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2
2704aedb5c21 bus: fsl-mc: dpio: add APIs for DPIO objects
0e6437941e44 bus: fsl-mc: dpio: add DPIO driver overview document

Do I have to resubmit?

Thanks,
Horia

Re: [PATCH v2] bus: fsl-mc: dpio: fix alter FQ state command

[Greg KH]

On Fri, Apr 28, 2017 at 10:47:14AM +0000, Horia Geantă wrote:
> On 4/28/2017 1:32 PM, Greg KH wrote:
> > On Sat, Apr 22, 2017 at 09:44:49AM +0300, Horia Geantă wrote:
> >> When checking the response verb, the valid bit should be masked out,
> >> since its value flips depending on what Response Register
> >> (RR0 /RR1) it's been read from.
> >>
> >> Fixes: 321eecb06bfb ("bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2")
> >> Signed-off-by: Horia Geantă <horia.geanta@nxp.com>
> >> ---
> >> v2: use QBMAN_RESULT_MASK instead of hard-coded mask
> > 
> > Why are you not using 'staging' in your subject line?
> > 
> I was not aware this is required and followed the statistics:
> 
> git log --oneline drivers/staging/fsl-mc/bus/dpio/
> 141a10aead9c staging: fsl-mc/dpio: Fix early writing of valid bit
> 993fec7e11ca bus: fsl-mc: dpio: add the DPAA2 DPIO object driver
> 780b626323d7 bus: fsl-mc: dpio: add the DPAA2 DPIO service interface
> 321eecb06bfb bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2
> 2704aedb5c21 bus: fsl-mc: dpio: add APIs for DPIO objects
> 0e6437941e44 bus: fsl-mc: dpio: add DPIO driver overview document
> 
> Do I have to resubmit?

No, I have edited the subject, it should be "staging: fsl-mc: bus: ..."
for future patches.

thanks,

greg k-h