[PATCH] RDMA/rxe: Fix error in rxe_task.c

[PATCH] RDMA/rxe: Fix error in rxe_task.c

[Bob Pearson]

In a previous patch TASKLET_STATE_SCHED was used as a bit but it
is a bit position instead. This patch corrects that error.

Reported-by: Dan Carpenter <error27@gmail.com>
Link: https://lore.kernel.org/linux-rdma/8a054b78-6d50-4bc6-8d8a-83f85fbdb82f@kili.mountain/
Fixes: d94671632572 ("RDMA/rxe: Rewrite rxe_task.c")
Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
 drivers/infiniband/sw/rxe/rxe_task.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_task.c b/drivers/infiniband/sw/rxe/rxe_task.c
index fea9a517c8d9..fb9a6bc8e620 100644
--- a/drivers/infiniband/sw/rxe/rxe_task.c
+++ b/drivers/infiniband/sw/rxe/rxe_task.c
@@ -21,7 +21,7 @@ static bool __reserve_if_idle(struct rxe_task *task)
 {
 	WARN_ON(rxe_read(task->qp) <= 0);
 
-	if (task->tasklet.state & TASKLET_STATE_SCHED)
+	if (task->tasklet.state & BIT(TASKLET_STATE_SCHED))
 		return false;
 
 	if (task->state == TASK_STATE_IDLE) {
@@ -46,7 +46,7 @@ static bool __reserve_if_idle(struct rxe_task *task)
  */
 static bool __is_done(struct rxe_task *task)
 {
-	if (task->tasklet.state & TASKLET_STATE_SCHED)
+	if (task->tasklet.state & BIT(TASKLET_STATE_SCHED))
 		return false;
 
 	if (task->state == TASK_STATE_IDLE ||
-- 
2.37.2

Re: [PATCH] RDMA/rxe: Fix error in rxe_task.c

[Leon Romanovsky]

On Wed, Mar 29, 2023 at 02:33:09PM -0500, Bob Pearson wrote:
> In a previous patch TASKLET_STATE_SCHED was used as a bit but it
> is a bit position instead. This patch corrects that error.
> 
> Reported-by: Dan Carpenter <error27@gmail.com>
> Link: https://lore.kernel.org/linux-rdma/8a054b78-6d50-4bc6-8d8a-83f85fbdb82f@kili.mountain/
> Fixes: d94671632572 ("RDMA/rxe: Rewrite rxe_task.c")
> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
> ---
>  drivers/infiniband/sw/rxe/rxe_task.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Like I said here https://lore.kernel.org/all/20230329191701.GG831478@unrealm 
Why didn't you used test_bit?

Also please fix your commit message and title to clearly say what this
patch is doing. "Fix error ..." is too broad.
https://lore.kernel.org/all/1a6376525c40454282a14ab659de0b17b02fe523.1680113901.git.leon@kernel.org/

Thanks

> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_task.c b/drivers/infiniband/sw/rxe/rxe_task.c
> index fea9a517c8d9..fb9a6bc8e620 100644
> --- a/drivers/infiniband/sw/rxe/rxe_task.c
> +++ b/drivers/infiniband/sw/rxe/rxe_task.c
> @@ -21,7 +21,7 @@ static bool __reserve_if_idle(struct rxe_task *task)
>  {
>  	WARN_ON(rxe_read(task->qp) <= 0);
>  
> -	if (task->tasklet.state & TASKLET_STATE_SCHED)
> +	if (task->tasklet.state & BIT(TASKLET_STATE_SCHED))
>  		return false;
>  
>  	if (task->state == TASK_STATE_IDLE) {
> @@ -46,7 +46,7 @@ static bool __reserve_if_idle(struct rxe_task *task)
>   */
>  static bool __is_done(struct rxe_task *task)
>  {
> -	if (task->tasklet.state & TASKLET_STATE_SCHED)
> +	if (task->tasklet.state & BIT(TASKLET_STATE_SCHED))
>  		return false;
>  
>  	if (task->state == TASK_STATE_IDLE ||
> -- 
> 2.37.2
> 

Re: [PATCH] RDMA/rxe: Fix error in rxe_task.c

[Bob Pearson]

On 3/29/23 14:33, Bob Pearson wrote:
> In a previous patch TASKLET_STATE_SCHED was used as a bit but it
> is a bit position instead. This patch corrects that error.
> 
> Reported-by: Dan Carpenter <error27@gmail.com>
> Link: https://lore.kernel.org/linux-rdma/8a054b78-6d50-4bc6-8d8a-83f85fbdb82f@kili.mountain/
> Fixes: d94671632572 ("RDMA/rxe: Rewrite rxe_task.c")
> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
> ---
>  drivers/infiniband/sw/rxe/rxe_task.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_task.c b/drivers/infiniband/sw/rxe/rxe_task.c
> index fea9a517c8d9..fb9a6bc8e620 100644
> --- a/drivers/infiniband/sw/rxe/rxe_task.c
> +++ b/drivers/infiniband/sw/rxe/rxe_task.c
> @@ -21,7 +21,7 @@ static bool __reserve_if_idle(struct rxe_task *task)
>  {
>  	WARN_ON(rxe_read(task->qp) <= 0);
>  
> -	if (task->tasklet.state & TASKLET_STATE_SCHED)
> +	if (task->tasklet.state & BIT(TASKLET_STATE_SCHED))
>  		return false;
>  
>  	if (task->state == TASK_STATE_IDLE) {
> @@ -46,7 +46,7 @@ static bool __reserve_if_idle(struct rxe_task *task)
>   */
>  static bool __is_done(struct rxe_task *task)
>  {
> -	if (task->tasklet.state & TASKLET_STATE_SCHED)
> +	if (task->tasklet.state & BIT(TASKLET_STATE_SCHED))
>  		return false;
>  
>  	if (task->state == TASK_STATE_IDLE ||

This patch fixes a bug in rxe_task.c introduced by the earlier patch (d94671632572 RDMA/rxe: Rewrite rxe_task.c)
which is in for-next. The bug actually has minimal effects because TASKLET_STATE_SCHED is zero and in testing
so far it doesn't seem to make a difference.

There is a second patch currently in patchworks ([for-next,v6] RDMA/rxe: Add workqueue support for tasks
[for-next,v6] RDMA/rxe: Add workqueue support for tasks 	- - - 	--- 	2023-03-02 	Bob Pearson New)
which is ahead of this one and replaces the tasklet implementation by work queues. This second patch replaces the
lines lines containing the error with a workqueue specific equivalent.

There are two ways forward here. We could fix the tasklet version by applying this patch first or ignore the
error and apply the workqueue patch first. My desire is to get rid of tasklets altogether so I prefer the
second choice. If we choose the first choice then we need to reorder the two patches in patchworks and
rebase the workqueue patch to match the fixed tasklet code.

Please suggest how you would like me to proceed.

Bob

Re: [PATCH] RDMA/rxe: Fix error in rxe_task.c

[Leon Romanovsky]

On Mon, Apr 03, 2023 at 03:38:13PM -0500, Bob Pearson wrote:
> On 3/29/23 14:33, Bob Pearson wrote:
> > In a previous patch TASKLET_STATE_SCHED was used as a bit but it
> > is a bit position instead. This patch corrects that error.
> > 
> > Reported-by: Dan Carpenter <error27@gmail.com>
> > Link: https://lore.kernel.org/linux-rdma/8a054b78-6d50-4bc6-8d8a-83f85fbdb82f@kili.mountain/
> > Fixes: d94671632572 ("RDMA/rxe: Rewrite rxe_task.c")
> > Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
> > ---
> >  drivers/infiniband/sw/rxe/rxe_task.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/infiniband/sw/rxe/rxe_task.c b/drivers/infiniband/sw/rxe/rxe_task.c
> > index fea9a517c8d9..fb9a6bc8e620 100644
> > --- a/drivers/infiniband/sw/rxe/rxe_task.c
> > +++ b/drivers/infiniband/sw/rxe/rxe_task.c
> > @@ -21,7 +21,7 @@ static bool __reserve_if_idle(struct rxe_task *task)
> >  {
> >  	WARN_ON(rxe_read(task->qp) <= 0);
> >  
> > -	if (task->tasklet.state & TASKLET_STATE_SCHED)
> > +	if (task->tasklet.state & BIT(TASKLET_STATE_SCHED))
> >  		return false;
> >  
> >  	if (task->state == TASK_STATE_IDLE) {
> > @@ -46,7 +46,7 @@ static bool __reserve_if_idle(struct rxe_task *task)
> >   */
> >  static bool __is_done(struct rxe_task *task)
> >  {
> > -	if (task->tasklet.state & TASKLET_STATE_SCHED)
> > +	if (task->tasklet.state & BIT(TASKLET_STATE_SCHED))
> >  		return false;
> >  
> >  	if (task->state == TASK_STATE_IDLE ||
> 
> This patch fixes a bug in rxe_task.c introduced by the earlier patch (d94671632572 RDMA/rxe: Rewrite rxe_task.c)
> which is in for-next. The bug actually has minimal effects because TASKLET_STATE_SCHED is zero and in testing
> so far it doesn't seem to make a difference.
> 
> There is a second patch currently in patchworks ([for-next,v6] RDMA/rxe: Add workqueue support for tasks
> [for-next,v6] RDMA/rxe: Add workqueue support for tasks 	- - - 	--- 	2023-03-02 	Bob Pearson New)
> which is ahead of this one and replaces the tasklet implementation by work queues. This second patch replaces the
> lines lines containing the error with a workqueue specific equivalent.
> 
> There are two ways forward here. We could fix the tasklet version by applying this patch first or ignore the
> error and apply the workqueue patch first. My desire is to get rid of tasklets altogether so I prefer the
> second choice. If we choose the first choice then we need to reorder the two patches in patchworks and
> rebase the workqueue patch to match the fixed tasklet code.
> 
> Please suggest how you would like me to proceed.

I prefer to have small fix first as it is unclear when "RDMA/rxe: Add workqueue support for tasks"
will be merged.
https://lore.kernel.org/all/20230330053933.GJ831478@unreal

Thanks

> 
> Bob

Re: [PATCH] RDMA/rxe: Fix error in rxe_task.c

[Bob Pearson]

On 4/3/23 23:58, Leon Romanovsky wrote:
> On Mon, Apr 03, 2023 at 03:38:13PM -0500, Bob Pearson wrote:
>> On 3/29/23 14:33, Bob Pearson wrote:
>>> In a previous patch TASKLET_STATE_SCHED was used as a bit but it
>>> is a bit position instead. This patch corrects that error.
>>>
>>> Reported-by: Dan Carpenter <error27@gmail.com>
>>> Link: https://lore.kernel.org/linux-rdma/8a054b78-6d50-4bc6-8d8a-83f85fbdb82f@kili.mountain/
>>> Fixes: d94671632572 ("RDMA/rxe: Rewrite rxe_task.c")
>>> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
>>> ---
>>>  drivers/infiniband/sw/rxe/rxe_task.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/infiniband/sw/rxe/rxe_task.c b/drivers/infiniband/sw/rxe/rxe_task.c
>>> index fea9a517c8d9..fb9a6bc8e620 100644
>>> --- a/drivers/infiniband/sw/rxe/rxe_task.c
>>> +++ b/drivers/infiniband/sw/rxe/rxe_task.c
>>> @@ -21,7 +21,7 @@ static bool __reserve_if_idle(struct rxe_task *task)
>>>  {
>>>  	WARN_ON(rxe_read(task->qp) <= 0);
>>>  
>>> -	if (task->tasklet.state & TASKLET_STATE_SCHED)
>>> +	if (task->tasklet.state & BIT(TASKLET_STATE_SCHED))
>>>  		return false;
>>>  
>>>  	if (task->state == TASK_STATE_IDLE) {
>>> @@ -46,7 +46,7 @@ static bool __reserve_if_idle(struct rxe_task *task)
>>>   */
>>>  static bool __is_done(struct rxe_task *task)
>>>  {
>>> -	if (task->tasklet.state & TASKLET_STATE_SCHED)
>>> +	if (task->tasklet.state & BIT(TASKLET_STATE_SCHED))
>>>  		return false;
>>>  
>>>  	if (task->state == TASK_STATE_IDLE ||
>>
>> This patch fixes a bug in rxe_task.c introduced by the earlier patch (d94671632572 RDMA/rxe: Rewrite rxe_task.c)
>> which is in for-next. The bug actually has minimal effects because TASKLET_STATE_SCHED is zero and in testing
>> so far it doesn't seem to make a difference.
>>
>> There is a second patch currently in patchworks ([for-next,v6] RDMA/rxe: Add workqueue support for tasks
>> [for-next,v6] RDMA/rxe: Add workqueue support for tasks 	- - - 	--- 	2023-03-02 	Bob Pearson New)
>> which is ahead of this one and replaces the tasklet implementation by work queues. This second patch replaces the
>> lines lines containing the error with a workqueue specific equivalent.
>>
>> There are two ways forward here. We could fix the tasklet version by applying this patch first or ignore the
>> error and apply the workqueue patch first. My desire is to get rid of tasklets altogether so I prefer the
>> second choice. If we choose the first choice then we need to reorder the two patches in patchworks and
>> rebase the workqueue patch to match the fixed tasklet code.
>>
>> Please suggest how you would like me to proceed.
> 
> I prefer to have small fix first as it is unclear when "RDMA/rxe: Add workqueue support for tasks"
> will be merged.
> https://lore.kernel.org/all/20230330053933.GJ831478@unreal
> 
> Thanks
> 
>>
>> Bob

Leon,

Then drop the workqueue patch and apply the fix rxe_task.c patch and I will resubmit the workqueue patch.

Thanks

Bob

Re: [PATCH] RDMA/rxe: Fix error in rxe_task.c

[Jason Gunthorpe]

On Wed, Mar 29, 2023 at 02:33:09PM -0500, Bob Pearson wrote:
> In a previous patch TASKLET_STATE_SCHED was used as a bit but it
> is a bit position instead. This patch corrects that error.
> 
> Reported-by: Dan Carpenter <error27@gmail.com>
> Link: https://lore.kernel.org/linux-rdma/8a054b78-6d50-4bc6-8d8a-83f85fbdb82f@kili.mountain/
> Fixes: d94671632572 ("RDMA/rxe: Rewrite rxe_task.c")
> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
> ---
>  drivers/infiniband/sw/rxe/rxe_task.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Applied to for-next

Thanks,
Jason