Re: [bug report] drm: Add support for the LogiCVC display controller

[Paul Kocialkowski <paul.kocialkowski@bootlin.com>]

[-- 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 --]