| Message ID | 20161209141944.22121-6-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Dec 09, 2016 at 03:19:43PM +0100, Daniel Vetter wrote: > We can be more clever than that and merge the 2 list walking loops. > It's all under the one mutex critical section anyway, so nothing funny > can happen. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_mode_config.c | 28 +++++++++++----------------- > 1 file changed, 11 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > index 64c2971478db..a6205a92dfee 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -84,7 +84,6 @@ 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; > @@ -105,25 +104,20 @@ 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++; > + copied = 0; > + fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr; > + list_for_each_entry(fb, &file_priv->fbs, filp_head) { > + fb_count++; > + if (fb_count > card_res->count_fbs) > + continue; > + > + if (put_user(fb->base.id, fb_id + copied)) { > + mutex_unlock(&file_priv->fbs_lock); > + return -EFAULT; > } > + copied++; if (fb_count < card_res->count_fbs && put_user(fb->base.id fb_id + fb_count) { } fb_count++; // no need for copied I'd just like for one style :) Otoh, this seems reasonable to apply to crtc/encoders/connectors as well, and on the other we already have a counter for the others. Hmm, setting count = copied in the previous patch is thereby wrong (change in uABI). -Chris
On Fri, Dec 09, 2016 at 09:57:45PM +0000, Chris Wilson wrote: > On Fri, Dec 09, 2016 at 03:19:43PM +0100, Daniel Vetter wrote: > > We can be more clever than that and merge the 2 list walking loops. > > It's all under the one mutex critical section anyway, so nothing funny > > can happen. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/drm_mode_config.c | 28 +++++++++++----------------- > > 1 file changed, 11 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > > index 64c2971478db..a6205a92dfee 100644 > > --- a/drivers/gpu/drm/drm_mode_config.c > > +++ b/drivers/gpu/drm/drm_mode_config.c > > @@ -84,7 +84,6 @@ 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; > > @@ -105,25 +104,20 @@ 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++; > > + copied = 0; > > + fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr; > > + list_for_each_entry(fb, &file_priv->fbs, filp_head) { > > + fb_count++; > > + if (fb_count > card_res->count_fbs) > > + continue; > > + > > + if (put_user(fb->base.id, fb_id + copied)) { > > + mutex_unlock(&file_priv->fbs_lock); > > + return -EFAULT; > > } > > + copied++; > > if (fb_count < card_res->count_fbs && > put_user(fb->base.id fb_id + fb_count) { > } > fb_count++; // no need for copied > > I'd just like for one style :) Otoh, this seems reasonable to apply to > crtc/encoders/connectors as well, and on the other we already have a > counter for the others. Hmm, setting count = copied in the previous > patch is thereby wrong (change in uABI). It's only a change if the connector count changed between when we read it and when we walked the list. But I guess you've convinced me that simplifying this loops more is a good idea, and this way we can use the same style for all of them. -Daniel
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 64c2971478db..a6205a92dfee 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -84,7 +84,6 @@ 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; @@ -105,25 +104,20 @@ 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++; + copied = 0; + fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr; + list_for_each_entry(fb, &file_priv->fbs, filp_head) { + fb_count++; + if (fb_count > card_res->count_fbs) + continue; + + if (put_user(fb->base.id, fb_id + copied)) { + mutex_unlock(&file_priv->fbs_lock); + return -EFAULT; } + copied++; } card_res->count_fbs = fb_count; mutex_unlock(&file_priv->fbs_lock);
We can be more clever than that and merge the 2 list walking loops. It's all under the one mutex critical section anyway, so nothing funny can happen. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_mode_config.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-)