Re: [bug report] libnvdimm: fix mishandled nvdimm_clear_poison() return value

[Dan Carpenter <error27@gmail.com>]

On Mon, Nov 28, 2022 at 10:24:07AM -0500, Jeff Moyer wrote:
> Dan Carpenter <error27@gmail.com> writes:
> 
> > Hello Dan Williams,
> >
> > The patch 868f036fee4b: "libnvdimm: fix mishandled
> > nvdimm_clear_poison() return value" from Dec 16, 2016, leads to the
> > following Smatch static checker warnings:
> >
> >     drivers/nvdimm/claim.c:287 nsio_rw_bytes() warn:
> >     replace divide condition 'cleared / 512' with 'cleared >= 512'
> >
> >     drivers/nvdimm/bus.c:210 nvdimm_account_cleared_poison() warn:
> >     replace divide condition 'cleared / 512' with 'cleared >= 512'
> >
> > drivers/nvdimm/claim.c
> >     252 static int nsio_rw_bytes(struct nd_namespace_common *ndns,
> >     253                 resource_size_t offset, void *buf, size_t size, int rw,
> >     254                 unsigned long flags)
> >     255 {
> >     256         struct nd_namespace_io *nsio = to_nd_namespace_io(&ndns->dev);
> >     257         unsigned int sz_align = ALIGN(size + (offset & (512 - 1)), 512);
> >     258         sector_t sector = offset >> 9;
> >     259         int rc = 0, ret = 0;
> >     260 
> >     261         if (unlikely(!size))
> >     262                 return 0;
> >     263 
> >     264         if (unlikely(offset + size > nsio->size)) {
> >     265                 dev_WARN_ONCE(&ndns->dev, 1, "request out of range\n");
> >     266                 return -EFAULT;
> >     267         }
> >     268 
> >     269         if (rw == READ) {
> >     270                 if (unlikely(is_bad_pmem(&nsio->bb, sector, sz_align)))
> >     271                         return -EIO;
> >     272                 if (copy_mc_to_kernel(buf, nsio->addr + offset, size) != 0)
> >     273                         return -EIO;
> >     274                 return 0;
> >     275         }
> >     276 
> >     277         if (unlikely(is_bad_pmem(&nsio->bb, sector, sz_align))) {
> >     278                 if (IS_ALIGNED(offset, 512) && IS_ALIGNED(size, 512)
> >     279                                 && !(flags & NVDIMM_IO_ATOMIC)) {
> >     280                         long cleared;
> >     281 
> >     282                         might_sleep();
> >     283                         cleared = nvdimm_clear_poison(&ndns->dev,
> >     284                                         nsio->res.start + offset, size);
> >     285                         if (cleared < size)
> >     286                                 rc = -EIO;
> > --> 287                         if (cleared > 0 && cleared / 512) {
> >                                                    ^^^^^^^^^^^^^
> > Smatch suggests changing this to "&& cleared >= 512" but it doesn't make
> > sense to say if (cleared > 0 && cleared >= 512) {.  Probably what was
> > instead intended was "if (cleared > 0 && (cleared % 512) == 0) {"?
> 
> No, it is correct as written.  cleared is the number of bytes cleared.
> The badblocks_clear interface takes 512 byte sectors as an input.  We
> only want to call badblocks_clear if we cleared /at least/ one sector.
> 
> It could probably use a comment, though.  :)

Okay.  Thanks for looking at this!

regards,
dan carpenter