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?
Thanks for the catch.
Paul
> --> 320 if (hoffset > LOGICVC_DIMENSIONS_MAX)
> 321 hoffset = LOGICVC_DIMENSIONS_MAX;
> 322
> 323 gap -= hoffset * layer_bytespp;
> 324 }
> 325
> 326 if (gap) {
> 327 drm_err(drm_dev,
> 328 "Unable to find layer %d buffer setup for 0x%x byte gap\n",
> 329 layer->index, fb_offset - base_offset);
> 330 return -EINVAL;
> 331 }
> 332
> 333 drm_dbg_kms(drm_dev, "Found layer %d buffer setup for 0x%x byte gap:\n",
> 334 layer->index, fb_offset - base_offset);
> 335
> 336 drm_dbg_kms(drm_dev, "- buffer_sel = 0x%x chunks of 0x%x bytes\n",
> 337 buffer_sel, buffer_offset);
> 338 drm_dbg_kms(drm_dev, "- voffset = 0x%x chunks of 0x%x bytes\n", voffset,
> 339 layer_stride);
> 340 drm_dbg_kms(drm_dev, "- hoffset = 0x%x chunks of 0x%x bytes\n", hoffset,
> 341 layer_bytespp);
> 342
> 343 if (setup) {
> 344 setup->buffer_sel = buffer_sel;
> 345 setup->voffset = voffset;
> 346 setup->hoffset = hoffset;
> 347 }
> 348
> 349 return 0;
> 350 }
>
> regards,
> dan carpenter
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Attachment:
signature.asc
Description: PGP signature
