| Message ID | 1443513413-28873-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 29 Sep 2015, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > With atomic drivers we need to make sure that (at least in general) > property reads hold the right locks. But the legacy dpms property is > special and can be read locklessly. Since userspace loves to just > randomly look at that all the time (like with "status") do that. > > To make it clear that we play tricks use the READ_ONCE compiler > barrier (and also for paranoia). > > Note that there's not really anything bad going on since even with the > new atomic paths we eventually end up not chasing any pointers (and > hence possibly freed memory and other fun stuff). The locking WARNING > has been added in > > commit 88a48e297b3a3bac6022c03babfb038f1a886cea > Author: Rob Clark <robdclark@gmail.com> > Date: Thu Dec 18 16:01:50 2014 -0500 > > drm: add atomic properties > > but since drivers are converting not everyone will have seen this from > the start. > > Jens reported this and submitted a patch to just grab the > mode_config.connection_mutex, but we can do a bit better. > > v2: Remove unused variables I failed to git add for real. > Reference: http://mid.gmane.org/20150928194822.GA3930@kernel.dk > Reported-by: Jens Axboe <axboe@fb.com> > Cc: Jens Axboe <axboe@fb.com> > Cc: Rob Clark <robdclark@gmail.com> > Cc: stable@vger.kernel.org > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_sysfs.c | 12 +++--------- > 1 file changed, 3 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > index f08873f6489c..615b7e667320 100644 > --- a/drivers/gpu/drm/drm_sysfs.c > +++ b/drivers/gpu/drm/drm_sysfs.c > @@ -230,18 +230,12 @@ static ssize_t dpms_show(struct device *device, > char *buf) > { > struct drm_connector *connector = to_drm_connector(device); > - struct drm_device *dev = connector->dev; > - uint64_t dpms_status; > - int ret; > + int dpms; > > - ret = drm_object_property_get_value(&connector->base, > - dev->mode_config.dpms_property, > - &dpms_status); > - if (ret) > - return 0; > + dpms = READ_ONCE(connector->dpms); > > return snprintf(buf, PAGE_SIZE, "%s\n", > - drm_get_dpms_name((int)dpms_status)); > + drm_get_dpms_name(dpms)); > } > > static ssize_t enabled_show(struct device *device, > -- > 2.5.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 09/29/2015 01:56 AM, Daniel Vetter wrote: > With atomic drivers we need to make sure that (at least in general) > property reads hold the right locks. But the legacy dpms property is > special and can be read locklessly. Since userspace loves to just > randomly look at that all the time (like with "status") do that. > > To make it clear that we play tricks use the READ_ONCE compiler > barrier (and also for paranoia). > > Note that there's not really anything bad going on since even with the > new atomic paths we eventually end up not chasing any pointers (and > hence possibly freed memory and other fun stuff). The locking WARNING > has been added in > > commit 88a48e297b3a3bac6022c03babfb038f1a886cea > Author: Rob Clark <robdclark@gmail.com> > Date: Thu Dec 18 16:01:50 2014 -0500 > > drm: add atomic properties > > but since drivers are converting not everyone will have seen this from > the start. > > Jens reported this and submitted a patch to just grab the > mode_config.connection_mutex, but we can do a bit better. > > v2: Remove unused variables I failed to git add for real. > > Reported-by: Jens Axboe <axboe@fb.com> > Cc: Jens Axboe <axboe@fb.com> > Cc: Rob Clark <robdclark@gmail.com> > Cc: stable@vger.kernel.org > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Works for me, thanks Daniel. Tested-by: Jens Axboe <axboe@fb.com>
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index f08873f6489c..615b7e667320 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -230,18 +230,12 @@ static ssize_t dpms_show(struct device *device, char *buf) { struct drm_connector *connector = to_drm_connector(device); - struct drm_device *dev = connector->dev; - uint64_t dpms_status; - int ret; + int dpms; - ret = drm_object_property_get_value(&connector->base, - dev->mode_config.dpms_property, - &dpms_status); - if (ret) - return 0; + dpms = READ_ONCE(connector->dpms); return snprintf(buf, PAGE_SIZE, "%s\n", - drm_get_dpms_name((int)dpms_status)); + drm_get_dpms_name(dpms)); } static ssize_t enabled_show(struct device *device,
With atomic drivers we need to make sure that (at least in general) property reads hold the right locks. But the legacy dpms property is special and can be read locklessly. Since userspace loves to just randomly look at that all the time (like with "status") do that. To make it clear that we play tricks use the READ_ONCE compiler barrier (and also for paranoia). Note that there's not really anything bad going on since even with the new atomic paths we eventually end up not chasing any pointers (and hence possibly freed memory and other fun stuff). The locking WARNING has been added in commit 88a48e297b3a3bac6022c03babfb038f1a886cea Author: Rob Clark <robdclark@gmail.com> Date: Thu Dec 18 16:01:50 2014 -0500 drm: add atomic properties but since drivers are converting not everyone will have seen this from the start. Jens reported this and submitted a patch to just grab the mode_config.connection_mutex, but we can do a bit better. v2: Remove unused variables I failed to git add for real. Reported-by: Jens Axboe <axboe@fb.com> Cc: Jens Axboe <axboe@fb.com> Cc: Rob Clark <robdclark@gmail.com> Cc: stable@vger.kernel.org Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_sysfs.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)