| Message ID | 20230329193308.7489-1-rpearsonhpe@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/rxe: Fix error in rxe_task.c | expand |
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 >
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
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
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
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
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 ||
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(-)