| Message ID | 20161211192019.29603-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Dec 11, 2016 at 08:20:19PM +0100, Daniel Vetter wrote: > Looping twice when we can do it once is silly. Also use a consistent > style. Note that there's a good race with the connector list walking, > since that is no longer protected by mode_config.mutex. But that's for > a later patch to fix. > > v2: Actually try to not blow up, somehow I lost the hunk that checks > we don't copy too much. Noticed by Chris. > > v3: > - squash all drm_mode_getresources cleanups into one > - use consistent style for walking objects (Chris) > > v4: > - Use u64_to_user_ptr (Chris) > - Don't forget to copy the last connector (Chris) > > v5: Chris was right ... > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_mode_config.c | 111 ++++++++++++++------------------------ > 1 file changed, 39 insertions(+), 72 deletions(-) > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > index 2735a5847ffa..b1e8bbceaf39 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -84,17 +84,11 @@ int drm_mode_getresources(struct drm_device *dev, void *data, > struct drm_file *file_priv) > { > struct drm_mode_card_res *card_res = data; > - struct list_head *lh; > struct drm_framebuffer *fb; > struct drm_connector *connector; > struct drm_crtc *crtc; > struct drm_encoder *encoder; > - int ret = 0; > - int connector_count = 0; > - int crtc_count = 0; > - int fb_count = 0; > - int encoder_count = 0; > - int copied = 0; > + int count, ret = 0; I'm down to a minor int but uABI uses u32. This being C, it all comes out in the wash. One day we may have -Wsign-compare, but not today! Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Sun, Dec 11, 2016 at 07:53:42PM +0000, Chris Wilson wrote: > On Sun, Dec 11, 2016 at 08:20:19PM +0100, Daniel Vetter wrote: > > Looping twice when we can do it once is silly. Also use a consistent > > style. Note that there's a good race with the connector list walking, > > since that is no longer protected by mode_config.mutex. But that's for > > a later patch to fix. > > > > v2: Actually try to not blow up, somehow I lost the hunk that checks > > we don't copy too much. Noticed by Chris. > > > > v3: > > - squash all drm_mode_getresources cleanups into one > > - use consistent style for walking objects (Chris) > > > > v4: > > - Use u64_to_user_ptr (Chris) > > - Don't forget to copy the last connector (Chris) > > > > v5: Chris was right ... And CI finally concurred that v5 works ;-) > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/drm_mode_config.c | 111 ++++++++++++++------------------------ > > 1 file changed, 39 insertions(+), 72 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > > index 2735a5847ffa..b1e8bbceaf39 100644 > > --- a/drivers/gpu/drm/drm_mode_config.c > > +++ b/drivers/gpu/drm/drm_mode_config.c > > @@ -84,17 +84,11 @@ int drm_mode_getresources(struct drm_device *dev, void *data, > > struct drm_file *file_priv) > > { > > struct drm_mode_card_res *card_res = data; > > - struct list_head *lh; > > struct drm_framebuffer *fb; > > struct drm_connector *connector; > > struct drm_crtc *crtc; > > struct drm_encoder *encoder; > > - int ret = 0; > > - int connector_count = 0; > > - int crtc_count = 0; > > - int fb_count = 0; > > - int encoder_count = 0; > > - int copied = 0; > > + int count, ret = 0; > > I'm down to a minor int but uABI uses u32. This being C, it all comes > out in the wash. One day we may have -Wsign-compare, but not today! > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Thanks for the review, applied to -misc. Can I volunteer you for 1-3 too please? Thanks, Daniel
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 2735a5847ffa..b1e8bbceaf39 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -84,17 +84,11 @@ int drm_mode_getresources(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_card_res *card_res = data; - struct list_head *lh; struct drm_framebuffer *fb; struct drm_connector *connector; struct drm_crtc *crtc; struct drm_encoder *encoder; - int ret = 0; - int connector_count = 0; - int crtc_count = 0; - int fb_count = 0; - int encoder_count = 0; - int copied = 0; + int count, ret = 0; uint32_t __user *fb_id; uint32_t __user *crtc_id; uint32_t __user *connector_id; @@ -105,89 +99,62 @@ int drm_mode_getresources(struct drm_device *dev, void *data, mutex_lock(&file_priv->fbs_lock); - /* - * For the non-control nodes we need to limit the list of resources - * by IDs in the group list for this node - */ - list_for_each(lh, &file_priv->fbs) - fb_count++; - - /* handle this in 4 parts */ - /* FBs */ - if (card_res->count_fbs >= fb_count) { - copied = 0; - fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr; - list_for_each_entry(fb, &file_priv->fbs, filp_head) { - if (put_user(fb->base.id, fb_id + copied)) { - mutex_unlock(&file_priv->fbs_lock); - return -EFAULT; - } - copied++; + count = 0; + fb_id = u64_to_user_ptr(card_res->fb_id_ptr); + list_for_each_entry(fb, &file_priv->fbs, filp_head) { + if (count < card_res->count_fbs && + put_user(fb->base.id, fb_id + count)) { + mutex_unlock(&file_priv->fbs_lock); + return -EFAULT; } + count++; } - card_res->count_fbs = fb_count; + card_res->count_fbs = count; mutex_unlock(&file_priv->fbs_lock); /* mode_config.mutex protects the connector list against e.g. DP MST * connector hot-adding. CRTC/Plane lists are invariant. */ mutex_lock(&dev->mode_config.mutex); - drm_for_each_crtc(crtc, dev) - crtc_count++; - - drm_for_each_connector(connector, dev) - connector_count++; - - drm_for_each_encoder(encoder, dev) - encoder_count++; - card_res->max_height = dev->mode_config.max_height; card_res->min_height = dev->mode_config.min_height; card_res->max_width = dev->mode_config.max_width; card_res->min_width = dev->mode_config.min_width; - /* CRTCs */ - if (card_res->count_crtcs >= crtc_count) { - copied = 0; - crtc_id = (uint32_t __user *)(unsigned long)card_res->crtc_id_ptr; - drm_for_each_crtc(crtc, dev) { - if (put_user(crtc->base.id, crtc_id + copied)) { - ret = -EFAULT; - goto out; - } - copied++; + count = 0; + crtc_id = u64_to_user_ptr(card_res->crtc_id_ptr); + drm_for_each_crtc(crtc, dev) { + if (count < card_res->count_crtcs && + put_user(crtc->base.id, crtc_id + count)) { + ret = -EFAULT; + goto out; } + count++; } - card_res->count_crtcs = crtc_count; - - /* Encoders */ - if (card_res->count_encoders >= encoder_count) { - copied = 0; - encoder_id = (uint32_t __user *)(unsigned long)card_res->encoder_id_ptr; - drm_for_each_encoder(encoder, dev) { - if (put_user(encoder->base.id, encoder_id + - copied)) { - ret = -EFAULT; - goto out; - } - copied++; + card_res->count_crtcs = count; + + count = 0; + encoder_id = u64_to_user_ptr(card_res->encoder_id_ptr); + drm_for_each_encoder(encoder, dev) { + if (count < card_res->count_encoders && + put_user(encoder->base.id, encoder_id + count)) { + ret = -EFAULT; + goto out; } + count++; } - card_res->count_encoders = encoder_count; - - /* Connectors */ - if (card_res->count_connectors >= connector_count) { - copied = 0; - connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr; - drm_for_each_connector(connector, dev) { - if (put_user(connector->base.id, - connector_id + copied)) { - ret = -EFAULT; - goto out; - } - copied++; + card_res->count_encoders = count; + + count = 0; + connector_id = u64_to_user_ptr(card_res->connector_id_ptr); + drm_for_each_connector(connector, dev) { + if (count < card_res->count_connectors && + put_user(connector->base.id, connector_id + count)) { + ret = -EFAULT; + goto out; } + count++; } - card_res->count_connectors = connector_count; + card_res->count_connectors = count; out: mutex_unlock(&dev->mode_config.mutex);
Looping twice when we can do it once is silly. Also use a consistent style. Note that there's a good race with the connector list walking, since that is no longer protected by mode_config.mutex. But that's for a later patch to fix. v2: Actually try to not blow up, somehow I lost the hunk that checks we don't copy too much. Noticed by Chris. v3: - squash all drm_mode_getresources cleanups into one - use consistent style for walking objects (Chris) v4: - Use u64_to_user_ptr (Chris) - Don't forget to copy the last connector (Chris) v5: Chris was right ... Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_mode_config.c | 111 ++++++++++++++------------------------ 1 file changed, 39 insertions(+), 72 deletions(-)