* [bug report] drm: Add support for the LogiCVC display controller
@ 2022-06-14 12:07 Dan Carpenter
2022-06-24 14:53 ` Paul Kocialkowski
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2022-06-14 12:07 UTC (permalink / raw)
To: paul.kocialkowski; +Cc: dri-devel
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"
--> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] drm: Add support for the LogiCVC display controller
2022-06-14 12:07 [bug report] drm: Add support for the LogiCVC display controller Dan Carpenter
@ 2022-06-24 14:53 ` Paul Kocialkowski
2022-06-27 5:26 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Paul Kocialkowski @ 2022-06-24 14:53 UTC (permalink / raw)
To: Dan Carpenter; +Cc: dri-devel
[-- Attachment #1: Type: text/plain, Size: 5546 bytes --]
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] drm: Add support for the LogiCVC display controller
2022-06-24 14:53 ` Paul Kocialkowski
@ 2022-06-27 5:26 ` Dan Carpenter
2022-06-27 7:31 ` Paul Kocialkowski
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2022-06-27 5:26 UTC (permalink / raw)
To: Paul Kocialkowski; +Cc: dri-devel
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.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] drm: Add support for the LogiCVC display controller
2022-06-27 5:26 ` Dan Carpenter
@ 2022-06-27 7:31 ` Paul Kocialkowski
0 siblings, 0 replies; 4+ messages in thread
From: Paul Kocialkowski @ 2022-06-27 7:31 UTC (permalink / raw)
To: Dan Carpenter; +Cc: dri-devel
[-- Attachment #1: Type: text/plain, Size: 4733 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-06-27 7:32 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-06-14 12:07 [bug report] drm: Add support for the LogiCVC display controller Dan Carpenter
2022-06-24 14:53 ` Paul Kocialkowski
2022-06-27 5:26 ` Dan Carpenter
2022-06-27 7:31 ` Paul Kocialkowski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.