Paul Mackerras writes: > Egbert Eich writes: > > > My patches support a wider range of chipsets (Matrox, R128 and Radeon) > > and provide a framework which makes it easy to add ioctl32 support > > to more chipsets. > > Unfortunately your macros have the effect of increasing the effort > required by a kernel developer to read and understand the code. In > the kernel we care much more about code being easy to read than about > it being easy to write. A kernel developer reading my code sees the > function names that they know (access_ok, copy_to_user, etc.) and can > see at a glance what the code is doing.
Right. This depends on the macros though. The ioctl32 code does the same kind of operation numerous times. The macros make the code less complex and allow the reader to focus on what conversions are done and not on how they are done. It is very unlikely that the macros do the wrong thing once they are set up right. However it's more likely that someone forgets to do a conversion. There are numerous other examples of macros in the DRM code which don't have this nice property. > > > Furthermore they have support for both the old style ioctl32 support > > and the one that uses compat_alloc_user_space() so they should work > > on a wider range of kernels. > > The biggest problem, though, is that you are still using > register_ioctl32_conversion, even for your "new" style support. That > function is about to be removed. We need to supply a compat_ioctl > function in the file_operations vector, which is what the code which > is now upstream does. That's fine. If somone tells me what needs to be done I'm happy to change this. I've been carrying this code along for too long now. When I started this this kind of conversion functions didn't exist. > > I would also like to discuss the treatment of handles a bit more. You > have taken the approach of always hashing handles. I had concluded > some time ago that that approach had problems because of the tendency > of the DRI userspace to use physical addresses (of the framebuffer and > card registers) as the offset in mmap calls (if I remember correctly, > it was a while ago that I last looked at this stuff). I'm interested > to know how your patch solves that problem. I found one instance of this left in the code. This was in the mga driver and would be easy to fix. In my opinion this is more a bug in the code. Still I've changed the hash function so that it will (most likely) pass values that fit into 32bit unmodified. The problem is that one has to find some number to 'tag' an SHM area. If this number is identical to a physical base address that is added later this physical area needs to receive a different tag. It may be possible to go back to and modify the hash function again a little bit more along the line of what Dave submitted to the kernel if there is consensus that this would make the case where a real physical base has been assigend to an SHM area already and this is not available any more even less likely. > > Also, does a kernel with your patch work with existing unmodified X > servers and DRI clients? On x86_64 that would mean a 64-bit X server > with 64-bit clients. Or do you require userspace to be updated if the > kernel DRM is updated with your patch? Haven't I mentined that it does? Handles that fit into 32bit should be handed to userspace unchanged, therefore if there is any code left that does arithmetic with these handles should continue to work. Handles that are used as real bases should be 32bit as these usually are pointer to base addresses. Egbert. ------------------------------------------------------- SF.Net email is sponsored by: Discover Easy Linux Migration Strategies from IBM. Find simple to follow Roadmaps, straightforward articles, informative Webcasts and more! Get everything you need to get up to speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click -- _______________________________________________ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
