Re: [bug report] drm: remove usage of drm_pci_alloc/free

[Joseph Kogut <joseph.kogut@gmail.com>]

Hi Dan,

On Fri, May 14, 2021 at 6:54 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hello Joseph Kogut,
>
> The patch 70556e24e18e: "drm: remove usage of drm_pci_alloc/free"
> from Apr 22, 2021, leads to the following static checker warning:
>
>     drivers/gpu/drm/drm_bufs.c:1090 drm_legacy_addbufs_pci()
>     warn: inconsistent returns '&dev->struct_mutex'.
>       Locked on  : 988
>       Unlocked on: 938,944,951,959,973,1005,1042,1060,1090
>
> drivers/gpu/drm/drm_bufs.c
>    965          temp_pagelist = kmalloc_array(dma->page_count + (count << page_order),
>    966                                        sizeof(*dma->pagelist),
>    967                                        GFP_KERNEL);
>    968          if (!temp_pagelist) {
>    969                  kfree(entry->buflist);
>    970                  kfree(entry->seglist);
>    971                  mutex_unlock(&dev->struct_mutex);
>    972                  atomic_dec(&dev->buf_alloc);
>    973                  return -ENOMEM;
>
> There is a bunch of clean up happenning on the existing returns.
>
>    974          }
>    975          memcpy(temp_pagelist,
>    976                 dma->pagelist, dma->page_count * sizeof(*dma->pagelist));
>    977          DRM_DEBUG("pagelist: %d entries\n",
>    978                    dma->page_count + (count << page_order));
>    979
>    980          entry->buf_size = size;
>    981          entry->page_order = page_order;
>    982          byte_count = 0;
>    983          page_count = 0;
>    984
>    985          while (entry->buf_count < count) {
>    986                  dmah = kmalloc(sizeof(drm_dma_handle_t), GFP_KERNEL);
>    987                  if (!dmah)
>    988                          return -ENOMEM;
>
> This new return has no clean up.  We a mutex_unlock(&dev->struct_mutex).
>
>    989
>    990                  dmah->size = total;
>    991                  dmah->vaddr = dma_alloc_coherent(dev->dev,
>    992                                                   dmah->size,
>    993                                                   &dmah->busaddr,
>    994                                                   GFP_KERNEL);
>    995                  if (!dmah->vaddr) {
>    996                          kfree(dmah);
>    997
>    998                          /* Set count correctly so we free the proper amount. */
>    999                          entry->buf_count = count;
>   1000                          entry->seg_count = count;
>   1001                          drm_cleanup_buf_error(dev, entry);
>   1002                          kfree(temp_pagelist);
>   1003                          mutex_unlock(&dev->struct_mutex);
>   1004                          atomic_dec(&dev->buf_alloc);
>   1005                          return -ENOMEM;
>   1006                  }
>   1007                  entry->seglist[entry->seg_count++] = dmah;
>   1008                  for (i = 0; i < (1 << page_order); i++) {
>   1009                          DRM_DEBUG("page %d @ 0x%08lx\n",
>
> regards,
> dan carpenter

Thanks for the report, I'll follow up with a patch to fix this.

Best,
Joseph