* [PATCH] ipc/sem: Remove redundant assignments @ 2022-04-09 10:19 Michal Orzel 2022-04-09 10:19 ` [PATCH] block: " Michal Orzel ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Michal Orzel @ 2022-04-09 10:19 UTC (permalink / raw) To: Nathan Chancellor, Nick Desaulniers, Tom Rix Cc: Michal Orzel, linux-kernel, llvm Get rid of redundant assignments which end up in values not being read either because they are overwritten or the function ends. Reported by clang-tidy [deadcode.DeadStores] Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com> --- ipc/sem.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/ipc/sem.c b/ipc/sem.c index 0dbdb98fdf2d..38ef91a63edd 100644 --- a/ipc/sem.c +++ b/ipc/sem.c @@ -766,7 +766,6 @@ static int perform_atomic_semop(struct sem_array *sma, struct sem_queue *q) for (sop = sops; sop < sops + nsops; sop++) { curr = &sma->sems[sop->sem_num]; sem_op = sop->sem_op; - result = curr->semval; if (sop->sem_flg & SEM_UNDO) { int undo = un->semadj[sop->sem_num] - sem_op; @@ -1430,7 +1429,6 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum, if (err) goto out_rcu_wakeup; - err = -EACCES; switch (cmd) { case GETALL: { -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] block: Remove redundant assignments 2022-04-09 10:19 [PATCH] ipc/sem: Remove redundant assignments Michal Orzel @ 2022-04-09 10:19 ` Michal Orzel 2022-04-09 12:01 ` Tom Rix 2022-04-09 11:47 ` [PATCH] ipc/sem: " Tom Rix 2022-04-11 15:47 ` Nathan Chancellor 2 siblings, 1 reply; 9+ messages in thread From: Michal Orzel @ 2022-04-09 10:19 UTC (permalink / raw) To: Jens Axboe, Richard Russon (FlatCap), Nathan Chancellor, Nick Desaulniers, Tom Rix Cc: Michal Orzel, linux-block, linux-kernel, linux-ntfs-dev, llvm Get rid of redundant assignments which end up in values not being read either because they are overwritten or the function ends. Reported by clang-tidy [deadcode.DeadStores] Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com> --- block/badblocks.c | 2 -- block/blk-map.c | 5 ++--- block/partitions/acorn.c | 4 ++-- block/partitions/atari.c | 1 - block/partitions/ldm.c | 15 +++------------ 5 files changed, 7 insertions(+), 20 deletions(-) diff --git a/block/badblocks.c b/block/badblocks.c index d39056630d9c..3afb550c0f7b 100644 --- a/block/badblocks.c +++ b/block/badblocks.c @@ -65,7 +65,6 @@ int badblocks_check(struct badblocks *bb, sector_t s, int sectors, s >>= bb->shift; target += (1<<bb->shift) - 1; target >>= bb->shift; - sectors = target - s; } /* 'target' is now the first block after the bad range */ @@ -345,7 +344,6 @@ int badblocks_clear(struct badblocks *bb, sector_t s, int sectors) s += (1<<bb->shift) - 1; s >>= bb->shift; target >>= bb->shift; - sectors = target - s; } write_seqlock_irq(&bb->lock); diff --git a/block/blk-map.c b/block/blk-map.c index c7f71d83eff1..fa72e63e18c2 100644 --- a/block/blk-map.c +++ b/block/blk-map.c @@ -260,10 +260,9 @@ static int bio_map_user_iov(struct request *rq, struct iov_iter *iter, npages = DIV_ROUND_UP(offs + bytes, PAGE_SIZE); - if (unlikely(offs & queue_dma_alignment(rq->q))) { - ret = -EINVAL; + if (unlikely(offs & queue_dma_alignment(rq->q))) j = 0; - } else { + else { for (j = 0; j < npages; j++) { struct page *page = pages[j]; unsigned int n = PAGE_SIZE - offs; diff --git a/block/partitions/acorn.c b/block/partitions/acorn.c index 2c381c694c57..d2fc122d7426 100644 --- a/block/partitions/acorn.c +++ b/block/partitions/acorn.c @@ -282,13 +282,13 @@ int adfspart_check_ADFS(struct parsed_partitions *state) #ifdef CONFIG_ACORN_PARTITION_RISCIX case PARTITION_RISCIX_SCSI: case PARTITION_RISCIX_MFM: - slot = riscix_partition(state, start_sect, slot, + riscix_partition(state, start_sect, slot, nr_sects); break; #endif case PARTITION_LINUX: - slot = linux_partition(state, start_sect, slot, + linux_partition(state, start_sect, slot, nr_sects); break; } diff --git a/block/partitions/atari.c b/block/partitions/atari.c index da5994175416..9655c728262a 100644 --- a/block/partitions/atari.c +++ b/block/partitions/atari.c @@ -140,7 +140,6 @@ int atari_partition(struct parsed_partitions *state) /* accept only GEM,BGM,RAW,LNX,SWP partitions */ if (!((pi->flg & 1) && OK_id(pi->id))) continue; - part_fmt = 2; put_partition (state, slot, be32_to_cpu(pi->st), be32_to_cpu(pi->siz)); diff --git a/block/partitions/ldm.c b/block/partitions/ldm.c index 27f6c7d9c776..38e58960ae03 100644 --- a/block/partitions/ldm.c +++ b/block/partitions/ldm.c @@ -736,7 +736,6 @@ static bool ldm_parse_cmp3 (const u8 *buffer, int buflen, struct vblk *vb) len = r_cols; } else { r_stripe = 0; - r_cols = 0; len = r_parent; } if (len < 0) @@ -783,11 +782,8 @@ static int ldm_parse_dgr3 (const u8 *buffer, int buflen, struct vblk *vb) r_id1 = ldm_relative (buffer, buflen, 0x24, r_diskid); r_id2 = ldm_relative (buffer, buflen, 0x24, r_id1); len = r_id2; - } else { - r_id1 = 0; - r_id2 = 0; + } else len = r_diskid; - } if (len < 0) return false; @@ -826,11 +822,8 @@ static bool ldm_parse_dgr4 (const u8 *buffer, int buflen, struct vblk *vb) r_id1 = ldm_relative (buffer, buflen, 0x44, r_name); r_id2 = ldm_relative (buffer, buflen, 0x44, r_id1); len = r_id2; - } else { - r_id1 = 0; - r_id2 = 0; + } else len = r_name; - } if (len < 0) return false; @@ -963,10 +956,8 @@ static bool ldm_parse_prt3(const u8 *buffer, int buflen, struct vblk *vb) return false; } len = r_index; - } else { - r_index = 0; + } else len = r_diskid; - } if (len < 0) { ldm_error("len %d < 0", len); return false; -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] block: Remove redundant assignments 2022-04-09 10:19 ` [PATCH] block: " Michal Orzel @ 2022-04-09 12:01 ` Tom Rix 2022-04-09 13:20 ` Michal Orzel 0 siblings, 1 reply; 9+ messages in thread From: Tom Rix @ 2022-04-09 12:01 UTC (permalink / raw) To: Michal Orzel, Jens Axboe, Richard Russon (FlatCap), Nathan Chancellor, Nick Desaulniers Cc: linux-block, linux-kernel, linux-ntfs-dev, llvm On 4/9/22 3:19 AM, Michal Orzel wrote: > Get rid of redundant assignments which end up in values not being > read either because they are overwritten or the function ends. This log is the same as your last patch. Instead of a general statement on deadstores, a more specific analysis of the setting being removed would be helpful. This will mean splitting the patch to match the analysis. Tom > > Reported by clang-tidy [deadcode.DeadStores] > > Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com> > --- > block/badblocks.c | 2 -- > block/blk-map.c | 5 ++--- > block/partitions/acorn.c | 4 ++-- > block/partitions/atari.c | 1 - > block/partitions/ldm.c | 15 +++------------ > 5 files changed, 7 insertions(+), 20 deletions(-) > > diff --git a/block/badblocks.c b/block/badblocks.c > index d39056630d9c..3afb550c0f7b 100644 > --- a/block/badblocks.c > +++ b/block/badblocks.c > @@ -65,7 +65,6 @@ int badblocks_check(struct badblocks *bb, sector_t s, int sectors, > s >>= bb->shift; > target += (1<<bb->shift) - 1; > target >>= bb->shift; > - sectors = target - s; > } > /* 'target' is now the first block after the bad range */ > > @@ -345,7 +344,6 @@ int badblocks_clear(struct badblocks *bb, sector_t s, int sectors) > s += (1<<bb->shift) - 1; > s >>= bb->shift; > target >>= bb->shift; > - sectors = target - s; > } > > write_seqlock_irq(&bb->lock); > diff --git a/block/blk-map.c b/block/blk-map.c > index c7f71d83eff1..fa72e63e18c2 100644 > --- a/block/blk-map.c > +++ b/block/blk-map.c > @@ -260,10 +260,9 @@ static int bio_map_user_iov(struct request *rq, struct iov_iter *iter, > > npages = DIV_ROUND_UP(offs + bytes, PAGE_SIZE); > > - if (unlikely(offs & queue_dma_alignment(rq->q))) { > - ret = -EINVAL; > + if (unlikely(offs & queue_dma_alignment(rq->q))) > j = 0; > - } else { > + else { > for (j = 0; j < npages; j++) { > struct page *page = pages[j]; > unsigned int n = PAGE_SIZE - offs; > diff --git a/block/partitions/acorn.c b/block/partitions/acorn.c > index 2c381c694c57..d2fc122d7426 100644 > --- a/block/partitions/acorn.c > +++ b/block/partitions/acorn.c > @@ -282,13 +282,13 @@ int adfspart_check_ADFS(struct parsed_partitions *state) > #ifdef CONFIG_ACORN_PARTITION_RISCIX > case PARTITION_RISCIX_SCSI: > case PARTITION_RISCIX_MFM: > - slot = riscix_partition(state, start_sect, slot, > + riscix_partition(state, start_sect, slot, > nr_sects); > break; > #endif > > case PARTITION_LINUX: > - slot = linux_partition(state, start_sect, slot, > + linux_partition(state, start_sect, slot, > nr_sects); > break; > } > diff --git a/block/partitions/atari.c b/block/partitions/atari.c > index da5994175416..9655c728262a 100644 > --- a/block/partitions/atari.c > +++ b/block/partitions/atari.c > @@ -140,7 +140,6 @@ int atari_partition(struct parsed_partitions *state) > /* accept only GEM,BGM,RAW,LNX,SWP partitions */ > if (!((pi->flg & 1) && OK_id(pi->id))) > continue; > - part_fmt = 2; > put_partition (state, slot, > be32_to_cpu(pi->st), > be32_to_cpu(pi->siz)); > diff --git a/block/partitions/ldm.c b/block/partitions/ldm.c > index 27f6c7d9c776..38e58960ae03 100644 > --- a/block/partitions/ldm.c > +++ b/block/partitions/ldm.c > @@ -736,7 +736,6 @@ static bool ldm_parse_cmp3 (const u8 *buffer, int buflen, struct vblk *vb) > len = r_cols; > } else { > r_stripe = 0; > - r_cols = 0; > len = r_parent; > } > if (len < 0) > @@ -783,11 +782,8 @@ static int ldm_parse_dgr3 (const u8 *buffer, int buflen, struct vblk *vb) > r_id1 = ldm_relative (buffer, buflen, 0x24, r_diskid); > r_id2 = ldm_relative (buffer, buflen, 0x24, r_id1); > len = r_id2; > - } else { > - r_id1 = 0; > - r_id2 = 0; > + } else > len = r_diskid; > - } > if (len < 0) > return false; > > @@ -826,11 +822,8 @@ static bool ldm_parse_dgr4 (const u8 *buffer, int buflen, struct vblk *vb) > r_id1 = ldm_relative (buffer, buflen, 0x44, r_name); > r_id2 = ldm_relative (buffer, buflen, 0x44, r_id1); > len = r_id2; > - } else { > - r_id1 = 0; > - r_id2 = 0; > + } else > len = r_name; > - } > if (len < 0) > return false; > > @@ -963,10 +956,8 @@ static bool ldm_parse_prt3(const u8 *buffer, int buflen, struct vblk *vb) > return false; > } > len = r_index; > - } else { > - r_index = 0; > + } else > len = r_diskid; > - } > if (len < 0) { > ldm_error("len %d < 0", len); > return false; ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] block: Remove redundant assignments 2022-04-09 12:01 ` Tom Rix @ 2022-04-09 13:20 ` Michal Orzel 2022-04-09 16:03 ` Tom Rix 0 siblings, 1 reply; 9+ messages in thread From: Michal Orzel @ 2022-04-09 13:20 UTC (permalink / raw) To: Tom Rix, Jens Axboe, Richard Russon (FlatCap), Nathan Chancellor, Nick Desaulniers Cc: linux-block, linux-kernel, linux-ntfs-dev, llvm Hi Tom, > > On 4/9/22 3:19 AM, Michal Orzel wrote: >> Get rid of redundant assignments which end up in values not being >> read either because they are overwritten or the function ends. > > This log is the same as your last patch. > > Instead of a general statement on deadstores, a more specific > > analysis of the setting being removed would be helpful. > > This will mean splitting the patch to match the analysis. > > Tom > I think that this explanation in a commit is enough for such a trivial patch. It informs that we are fixing clang-tidy findings related to deadstores in a block subsystem. What analysis would you want to see? Something like "Remove deadstore assignment ret = -EINVAL in a function bio_map_user_iov" ? I think that it will create too much of not needed overhead. Cheers, Michal ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] block: Remove redundant assignments 2022-04-09 13:20 ` Michal Orzel @ 2022-04-09 16:03 ` Tom Rix 2022-04-12 15:23 ` Michal Orzel 0 siblings, 1 reply; 9+ messages in thread From: Tom Rix @ 2022-04-09 16:03 UTC (permalink / raw) To: Michal Orzel, Jens Axboe, Richard Russon (FlatCap), Nathan Chancellor, Nick Desaulniers Cc: linux-block, linux-kernel, linux-ntfs-dev, llvm On 4/9/22 6:20 AM, Michal Orzel wrote: > Hi Tom, > >> On 4/9/22 3:19 AM, Michal Orzel wrote: >>> Get rid of redundant assignments which end up in values not being >>> read either because they are overwritten or the function ends. >> This log is the same as your last patch. >> >> Instead of a general statement on deadstores, a more specific >> >> analysis of the setting being removed would be helpful. >> >> This will mean splitting the patch to match the analysis. >> >> Tom >> > I think that this explanation in a commit is enough for such a trivial patch. > It informs that we are fixing clang-tidy findings related to deadstores > in a block subsystem. What analysis would you want to see? > Something like "Remove deadstore assignment ret = -EINVAL in a function bio_map_user_iov" ? > I think that it will create too much of not needed overhead. The compiler should remove these already. All of the static analyzers produce false positives, if you do not provide analysis the reviewer has to assume you have not done any and has to do the false positive analysis themselves. This burdens the reviewer for something of small value that can be dismissed as churn. Tom > > Cheers, > Michal > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] block: Remove redundant assignments 2022-04-09 16:03 ` Tom Rix @ 2022-04-12 15:23 ` Michal Orzel 0 siblings, 0 replies; 9+ messages in thread From: Michal Orzel @ 2022-04-12 15:23 UTC (permalink / raw) To: Tom Rix, Jens Axboe, Richard Russon (FlatCap), Nathan Chancellor, Nick Desaulniers Cc: linux-block, linux-kernel, linux-ntfs-dev, llvm Hi Tom, On 09.04.2022 18:03, Tom Rix wrote: > > On 4/9/22 6:20 AM, Michal Orzel wrote: >> Hi Tom, >> >>> On 4/9/22 3:19 AM, Michal Orzel wrote: >>>> Get rid of redundant assignments which end up in values not being >>>> read either because they are overwritten or the function ends. >>> This log is the same as your last patch. >>> >>> Instead of a general statement on deadstores, a more specific >>> >>> analysis of the setting being removed would be helpful. >>> >>> This will mean splitting the patch to match the analysis. >>> >>> Tom >>> >> I think that this explanation in a commit is enough for such a trivial patch. >> It informs that we are fixing clang-tidy findings related to deadstores >> in a block subsystem. What analysis would you want to see? >> Something like "Remove deadstore assignment ret = -EINVAL in a function bio_map_user_iov" ? >> I think that it will create too much of not needed overhead. > > The compiler should remove these already. All of the static analyzers produce false positives, if you do not provide analysis the reviewer has to assume you have not done any and has to do the false positive analysis themselves. This burdens the reviewer for something of small value that can be dismissed as churn. > > Tom > Ok, I will split the patch into series in v2 and I will add some analysis into commit msg. Cheers, Michal ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ipc/sem: Remove redundant assignments 2022-04-09 10:19 [PATCH] ipc/sem: Remove redundant assignments Michal Orzel 2022-04-09 10:19 ` [PATCH] block: " Michal Orzel @ 2022-04-09 11:47 ` Tom Rix 2022-04-11 15:47 ` Nathan Chancellor 2 siblings, 0 replies; 9+ messages in thread From: Tom Rix @ 2022-04-09 11:47 UTC (permalink / raw) To: Michal Orzel, Nathan Chancellor, Nick Desaulniers; +Cc: linux-kernel, llvm On 4/9/22 3:19 AM, Michal Orzel wrote: > Get rid of redundant assignments which end up in values not being > read either because they are overwritten or the function ends. > > Reported by clang-tidy [deadcode.DeadStores] > > Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com> Reviewed-by: Tom Rix <trix@redhat.com> > --- > ipc/sem.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/ipc/sem.c b/ipc/sem.c > index 0dbdb98fdf2d..38ef91a63edd 100644 > --- a/ipc/sem.c > +++ b/ipc/sem.c > @@ -766,7 +766,6 @@ static int perform_atomic_semop(struct sem_array *sma, struct sem_queue *q) > for (sop = sops; sop < sops + nsops; sop++) { > curr = &sma->sems[sop->sem_num]; > sem_op = sop->sem_op; > - result = curr->semval; > > if (sop->sem_flg & SEM_UNDO) { > int undo = un->semadj[sop->sem_num] - sem_op; > @@ -1430,7 +1429,6 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum, > if (err) > goto out_rcu_wakeup; > > - err = -EACCES; > switch (cmd) { > case GETALL: > { ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ipc/sem: Remove redundant assignments 2022-04-09 10:19 [PATCH] ipc/sem: Remove redundant assignments Michal Orzel 2022-04-09 10:19 ` [PATCH] block: " Michal Orzel 2022-04-09 11:47 ` [PATCH] ipc/sem: " Tom Rix @ 2022-04-11 15:47 ` Nathan Chancellor 2022-04-12 15:19 ` Michal Orzel 2 siblings, 1 reply; 9+ messages in thread From: Nathan Chancellor @ 2022-04-11 15:47 UTC (permalink / raw) To: Michal Orzel; +Cc: Nick Desaulniers, Tom Rix, linux-kernel, llvm, Andrew Morton Hi Michal, On Sat, Apr 09, 2022 at 12:19:31PM +0200, Michal Orzel wrote: > Get rid of redundant assignments which end up in values not being > read either because they are overwritten or the function ends. > > Reported by clang-tidy [deadcode.DeadStores] > > Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com> Thank you for the patch! This should go through Andrew Morton, now CC'd. For the future, having some history behind the origin of these warnings would make review a little easier. I'll comment on each below. Reviewed-by: Nathan Chancellor <nathan@kernel.org> > --- > ipc/sem.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/ipc/sem.c b/ipc/sem.c > index 0dbdb98fdf2d..38ef91a63edd 100644 > --- a/ipc/sem.c > +++ b/ipc/sem.c > @@ -766,7 +766,6 @@ static int perform_atomic_semop(struct sem_array *sma, struct sem_queue *q) > for (sop = sops; sop < sops + nsops; sop++) { > curr = &sma->sems[sop->sem_num]; > sem_op = sop->sem_op; > - result = curr->semval; Caused by commit 4ce33ec2e42d ("ipc/sem: optimize perform_atomic_semop()"). Seems like a simple copy and paste bug. > if (sop->sem_flg & SEM_UNDO) { > int undo = un->semadj[sop->sem_num] - sem_op; > @@ -1430,7 +1429,6 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum, > if (err) > goto out_rcu_wakeup; > > - err = -EACCES; I was not able to figure out this one but it does look correct, as err is either set properly in the switch statement below or we skip over it to the 'err = -EINVAL'. > switch (cmd) { > case GETALL: > { > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ipc/sem: Remove redundant assignments 2022-04-11 15:47 ` Nathan Chancellor @ 2022-04-12 15:19 ` Michal Orzel 0 siblings, 0 replies; 9+ messages in thread From: Michal Orzel @ 2022-04-12 15:19 UTC (permalink / raw) To: Nathan Chancellor Cc: Nick Desaulniers, Tom Rix, linux-kernel, llvm, Andrew Morton Hi Nathan, On 11.04.2022 17:47, Nathan Chancellor wrote: > Hi Michal, > > On Sat, Apr 09, 2022 at 12:19:31PM +0200, Michal Orzel wrote: >> Get rid of redundant assignments which end up in values not being >> read either because they are overwritten or the function ends. >> >> Reported by clang-tidy [deadcode.DeadStores] >> >> Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com> > > Thank you for the patch! This should go through Andrew Morton, now CC'd. > > For the future, having some history behind the origin of these warnings > would make review a little easier. I'll comment on each below. > > Reviewed-by: Nathan Chancellor <nathan@kernel.org> > Thanks, I will keep that in mind when sending other patches fixing static analyzer findings. Cheers, Michal ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-04-12 15:24 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-09 10:19 [PATCH] ipc/sem: Remove redundant assignments Michal Orzel 2022-04-09 10:19 ` [PATCH] block: " Michal Orzel 2022-04-09 12:01 ` Tom Rix 2022-04-09 13:20 ` Michal Orzel 2022-04-09 16:03 ` Tom Rix 2022-04-12 15:23 ` Michal Orzel 2022-04-09 11:47 ` [PATCH] ipc/sem: " Tom Rix 2022-04-11 15:47 ` Nathan Chancellor 2022-04-12 15:19 ` Michal Orzel
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.