On Sat 30-03-13 18:26:53, Ilija Hadzic wrote: > This looks a bit like a hack and it doesn't look right, > conceptually. If the call fails, it should restore things as if > nothing has ever happened and overwriting old_mapping is not going to > do the trick. OK, I thought this is what the patch does as it falls back to &inode->i_data which is the default mapping for all inodes or it uses what used to be in device mapping. I am obviously not familiar with the drm code but it feels a bit strange that the device mapping can be different than inode's resp. file's one and even more confusing that inode and file are saved separately. > I think the right way to fix it would be to separately store the > original mapping for filp->f_mapping and inode->i_mapping and restore > it from their respective temporary variables if drm_open_helper or > drm_setup fail. Attached is a quick patch to show you [...] > @@ -137,6 +139,8 @@ int drm_open(struct inode *inode, struct file *filp) > if (!dev->open_count++) > need_setup = 1; > mutex_lock(&dev->struct_mutex); > + old_fmapping = filp->f_mapping; > + old_imapping = inode->i_mapping; How can file and inode mappings be different? > old_mapping = dev->dev_mapping; > if (old_mapping == NULL) > dev->dev_mapping = &inode->i_data; > @@ -159,8 +163,8 @@ int drm_open(struct inode *inode, struct file *filp) > > err_undo: > mutex_lock(&dev->struct_mutex); > - filp->f_mapping = old_mapping; > - inode->i_mapping = old_mapping; > + filp->f_mapping = old_fmapping; > + inode->i_mapping = old_imapping; > iput(container_of(dev->dev_mapping, struct inode, i_data)); > dev->dev_mapping = old_mapping; > mutex_unlock(&dev->struct_mutex); -- 1.8.1.5 -- Michal Hocko SUSE Labs _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
