Re: [bug report] drm: Add support for the LogiCVC display controller
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 = >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
Re: [bug report] drm: Add support for the LogiCVC display controller
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 = >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.
regards,
dan carpenter
Re: [bug report] drm: Add support for the LogiCVC display controller
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 = >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
