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. :) Cheers, Jeff > > 288 cleared /= 512; > 289 badblocks_clear(&nsio->bb, sector, > cleared); > 290 } > 291 arch_invalidate_pmem(nsio->addr + offset, > size); > 292 } else > 293 rc = -EIO; > 294 } > 295 > 296 memcpy_flushcache(nsio->addr + offset, buf, size); > 297 ret = nvdimm_flush(to_nd_region(ndns->dev.parent), NULL); > 298 if (ret) > 299 rc = ret; > 300 > 301 return rc; > 302 } > > regards, > dan carpenter
