On Mon, Nov 28, 2022 at 10:24:07AM -0500, Jeff Moyer wrote: > Dan Carpenter <[email protected]> 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
