Hi Dan, On Mon 27 Jun 22, 08:26, Dan Carpenter wrote: > On Fri, Jun 24, 2022 at 04:53:25PM +0200, Paul Kocialkowski wrote: > > Hello Dan, > > > > On Tue 14 Jun 22, 15:07, Dan Carpenter wrote: > > > Hello Paul Kocialkowski, > > > > > > The patch efeeaefe9be5: "drm: Add support for the LogiCVC display > > > controller" from May 20, 2022, leads to the following Smatch static > > > checker warning: > > > > > > drivers/gpu/drm/logicvc/logicvc_layer.c:320 > > > logicvc_layer_buffer_find_setup() > > > warn: impossible condition '(hoffset > (((((1))) << (16)) - 1)) => > > > (0-u16max > u16max)' > > > > > > drivers/gpu/drm/logicvc/logicvc_layer.c > > > 258 int logicvc_layer_buffer_find_setup(struct logicvc_drm *logicvc, > > > 259 struct logicvc_layer *layer, > > > 260 struct drm_plane_state *state, > > > 261 struct > > > logicvc_layer_buffer_setup *setup) > > > 262 { > > > 263 struct drm_device *drm_dev = &logicvc->drm_dev; > > > 264 struct drm_framebuffer *fb = state->fb; > > > 265 /* All the supported formats have a single data plane. */ > > > 266 u32 layer_bytespp = fb->format->cpp[0]; > > > 267 u32 layer_stride = layer_bytespp * > > > logicvc->config.row_stride; > > > 268 u32 base_offset = layer->config.base_offset * > > > layer_stride; > > > 269 u32 buffer_offset = layer->config.buffer_offset * > > > layer_stride; > > > 270 u8 buffer_sel = 0; > > > 271 u16 voffset = 0; > > > 272 u16 hoffset = 0; > > > 273 phys_addr_t fb_addr; > > > 274 u32 fb_offset; > > > 275 u32 gap; > > > 276 > > > 277 if (!logicvc->reserved_mem_base) { > > > 278 drm_err(drm_dev, "No reserved memory base was > > > registered!\n"); > > > 279 return -ENOMEM; > > > 280 } > > > 281 > > > 282 fb_addr = drm_fb_cma_get_gem_addr(fb, state, 0); > > > 283 if (fb_addr < logicvc->reserved_mem_base) { > > > 284 drm_err(drm_dev, > > > 285 "Framebuffer memory below reserved memory > > > base!\n"); > > > 286 return -EINVAL; > > > 287 } > > > 288 > > > 289 fb_offset = (u32) (fb_addr - logicvc->reserved_mem_base); > > > 290 > > > 291 if (fb_offset < base_offset) { > > > 292 drm_err(drm_dev, > > > 293 "Framebuffer offset below layer base > > > offset!\n"); > > > 294 return -EINVAL; > > > 295 } > > > 296 > > > 297 gap = fb_offset - base_offset; > > > 298 > > > 299 /* Use the possible video buffers selection. */ > > > 300 if (gap && buffer_offset) { > > > 301 buffer_sel = gap / buffer_offset; > > > 302 if (buffer_sel > LOGICVC_BUFFER_SEL_MAX) > > > 303 buffer_sel = LOGICVC_BUFFER_SEL_MAX; > > > 304 > > > 305 gap -= buffer_sel * buffer_offset; > > > 306 } > > > 307 > > > 308 /* Use the vertical offset. */ > > > 309 if (gap && layer_stride && > > > logicvc->config.layers_configurable) { > > > 310 voffset = gap / layer_stride; > > > 311 if (voffset > LOGICVC_LAYER_VOFFSET_MAX) > > > 312 voffset = LOGICVC_LAYER_VOFFSET_MAX; > > > 313 > > > 314 gap -= voffset * layer_stride; > > > 315 } > > > 316 > > > 317 /* Use the horizontal offset. */ > > > 318 if (gap && layer_bytespp && > > > logicvc->config.layers_configurable) { > > > 319 hoffset = gap / layer_bytespp; > > > > > > Can "gap / layer_bytespp" ever be more than USHRT_MAX? Because if so > > > that won't fit into "hoffset" > > > > Well there is nothing that really restricts the size of the gap, so yes this > > could happen. At this stage the gap should have been reduced already but we > > never really know. > > > > Would it make sense to add a check that gap / layer_bytespp <= USHRT_MAX > > in that if statement? > > My favorite fix would be to declare "hoffset" as a unsigned int.
Well the register itself only has 16 bits available for the value, so there would still be a problem in that situation. What do you think? Paul -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com
signature.asc
Description: PGP signature