[PATCH] staging: vc04_services: Add 32-bit compatibility ioctls

[Michael Zoran]

On Tue, 2016-11-08 at 13:23 -0800, Eric Anholt wrote:
> Michael Zoran <mzoran at crowfest.net> writes:
> 
> > On Tue, 2016-11-08 at 13:11 +0100, Arnd Bergmann wrote:
> > > On Monday, November 7, 2016 4:48:35 PM CET Michael Zoran wrote:
> > > >  .../vc04_services/interface/vchiq_arm/vchiq_arm.c  | 269
> > > > +++++++++++++++++++++
> > > >  .../vc04_services/interface/vchiq_arm/vchiq_if.h   |  25 ++
> > > >  .../interface/vchiq_arm/vchiq_ioctl.h              | 102
> > > > ++++++++
> > > >  3 files changed, 396 insertions(+)
> > > > 
> > > > diff --git
> > > > a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > index 8fcd940..df343a0 100644
> > > > ---
> > > > a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > +++
> > > > b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > @@ -573,12 +573,40 @@ vchiq_ioctl(struct file *file, unsigned
> > > > int
> > > > cmd, unsigned long arg)
> > > >  				"vchiq: could not connect:
> > > > %d",
> > > > status);
> > > >  		break;
> > > >  
> > > > +#if defined(CONFIG_64BIT)
> > > > +	case VCHIQ_IOC_CREATE_SERVICE32:
> > > > +#endif
> > > >  	case VCHIQ_IOC_CREATE_SERVICE: {
> > > >  		VCHIQ_CREATE_SERVICE_T args;
> > > >  		USER_SERVICE_T *user_service = NULL;
> > > >  		void *userdata;
> > > >  		int srvstate;
> > > >  
> > > > +#if defined(CONFIG_64BIT)
> > > > +		if (cmd == VCHIQ_IOC_CREATE_SERVICE32) {
> > > 
> > > Better use CONFIG_COMPAT here. Also, a simple #ifdef is
> > > sufficient
> > > as neither of those symbols can be a loadable module.
> > > 
> > 
> > OK, I can clean that up and resubmit.
> > 
> > > Also, just move all the compat handling into the .compat_ioctl
> > > callback function and move out the common parts into helpers
> > > for simplicity.
> > > 
> > 
> > That's a bit tricky.  Hopefully GregK will respond if he'll willing
> > to approve a change that rewrites the whole ioctl path.
> 
> Another way of doing compat ioctls that doesn't rewrite your whole
> non-compat ioctl path is in drivers/gpu/drm/drm_ioc32.c: the compat
> path
> just rewrites the 32-bit ioctl into a 64-bit struct that it passes to
> the normal ioctl handler.

That really is a cool way to do 32-bit compat. I didn't know it was
possible to do it that way.

In this case, the native ioctl mess is so bad that IMHO it's not
maintainable. Perhaps it is better to fix the mess now while the whole
driver is still marked "broken" and most of the patches are still in
linux-next.