On Mon, Aug 16, 2021 at 12:31 PM Desmond Cheong Zhi Xi <desmondcheongzx@xxxxxxxxx> wrote: > > On 16/8/21 5:04 pm, Daniel Vetter wrote: > > On Mon, Aug 16, 2021 at 10:53 AM Desmond Cheong Zhi Xi > > <desmondcheongzx@xxxxxxxxx> wrote: > >> On 16/8/21 2:47 am, kernel test robot wrote: > >>> Hi Desmond, > >>> > >>> Thank you for the patch! Yet something to improve: > >>> > >>> [auto build test ERROR on next-20210813] > >>> [also build test ERROR on v5.14-rc5] > >>> [cannot apply to linus/master v5.14-rc5 v5.14-rc4 v5.14-rc3] > >>> [If your patch is applied to the wrong git tree, kindly drop us a note. > >>> And when submitting patch, we suggest to use '--base' as documented in > >>> https://git-scm.com/docs/git-format-patch] > >>> > >>> url: https://github.com/0day-ci/linux/commits/Desmond-Cheong-Zhi-Xi/drm-avoid-races-with-modesetting-rights/20210815-234145 > >>> base: 4b358aabb93a2c654cd1dcab1a25a589f6e2b153 > >>> config: i386-randconfig-a004-20210815 (attached as .config) > >>> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 > >>> reproduce (this is a W=1 build): > >>> # https://github.com/0day-ci/linux/commit/cf6d8354b7d7953cd866fad004cbb189adfa074f > >>> git remote add linux-review https://github.com/0day-ci/linux > >>> git fetch --no-tags linux-review Desmond-Cheong-Zhi-Xi/drm-avoid-races-with-modesetting-rights/20210815-234145 > >>> git checkout cf6d8354b7d7953cd866fad004cbb189adfa074f > >>> # save the attached .config to linux build tree > >>> make W=1 ARCH=i386 > >>> > >>> If you fix the issue, kindly add following tag as appropriate > >>> Reported-by: kernel test robot <lkp@xxxxxxxxx> > >>> > >>> All errors (new ones prefixed by >>, old ones prefixed by <<): > >>> > >>>>> ERROR: modpost: "task_work_add" [drivers/gpu/drm/drm.ko] undefined! > >>> > >> > >> I'm a bit uncertain about this. Looking into the .config used, this > >> error seems to happen because task_work_add isn't an exported symbol, > >> but DRM is being compiled as a loadable kernel module (CONFIG_DRM=m). > >> > >> One way to deal with this is to export the symbol, but there was a > >> proposed patch to do this a few months back that wasn't picked up [1], > >> so I'm not sure what to make of this. > >> > >> I'll export the symbol as part of a v3 series, and check in with the > >> task-work maintainers. > >> > >> Link: > >> https://lore.kernel.org/lkml/20210127150029.13766-3-joshi.k@xxxxxxxxxxx/ [1] > > > > Yeah that sounds best. I have two more thoughts on the patch: > > - drm_master_flush isn't used by any modules outside of drm.ko, so we > > can unexport it and drop the kerneldoc (the comment is still good). > > These kind of internal functions have their declaration in > > drm-internal.h - there's already a few there from drm_auth.c > > > > Sounds good, I'll do that and move the declaration from drm_auth.h to > drm_internal.h. > > > - We know have 3 locks for master state, that feels a bit like > > overkill. The spinlock I think we need to keep due to lock inversions, > > but the master_mutex and master_rwsem look like we should be able to > > merge them? I.e. anywhere we currently grab the master_mutex we could > > instead grab the rwsem in either write mode (when we change stuff) or > > read mode (when we just check, like in master_internal_acquire). > > > > Thoughts? > > -Daniel > > > > Using rwsem in the places where we currently hold the mutex seems pretty > doable. > > There are some tricky bits once we add rwsem read locks to the ioctl > handler. Some ioctl functions like drm_authmagic need a write lock. Ah yes, I only looked at the dropmaster/setmaster ioctl, and those don't have the DRM_MASTER bit set. > In this particular case, it might make sense to break master_mutex down > into finer-grained locks, since the function doesn't change master > permissions. It just needs to prevent concurrent writes to the > drm_master.magic_map idr. Yeah for authmagic we could perhaps just reuse the spinlock to protect ->magic_map? > For other ioctls, I'll take a closer look on a case-by-case basis. If it's too much shuffling then I think totally fine to leave things as-is. Just feels a bit silly to have 3 locks, on of which is an rwlock itself, for this fairly small amount of state. -Daniel > > >> > >>> --- > >>> 0-DAY CI Kernel Test Service, Intel Corporation > >>> https://lists.01.org/hyperkitty/list/kbuild-all@xxxxxxxxxxxx > >>> > >> > > > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
