Add FBUNSUPPORTED ioctl for bcm2708_fb

[wuyuehang]

since fbturbo introduces and use it on copyarea operations for
unsupported dummy ioctl which is expected to return failure.
In order not to confuse user in kernel log, we should filter it out.

240 Nov 16 02:53:15 raspberrypi kernel: [ 7.522998] smsc95xx 1-1.1:1.0 eth0: link up, 100Mbps, full-duplex, lpa 0x43E1
241 Nov 16 02:53:15 raspberrypi kernel: [ 8.343603] Adding 102396k swap on /var/swap. Priority:-1 extents:5 across:573436k SSFS
242 Nov 16 02:53:16 raspberrypi kernel: [ 9.572984] bcm2708_fb soc:fb: Unknown ioctl 0x40187a22
243 Nov 16 02:53:17 raspberrypi kernel: [ 10.465163] cfg80211: Calling CRDA to update world regulatory domain
244 Nov 16 02:53:20 raspberrypi kernel: [ 13.615097] cfg80211: Calling CRDA to update world regulatory domain
245 Nov 16 02:53:23 raspberrypi kernel: [ 16.765045] cfg80211: Calling CRDA to update world regulatory domain

related fbturbo copyarea src code at: https://github.com/ssvb/xf86-video-fbturbo/blob/master/src/fb_copyarea.c#L76

[notro]

I'm wondering if we should just turn dev_err() into dev_dbg() instead and silence the error that way. Let userspace report the error if necessary.
Let's see what @popcornmix thinks.

[wuyuehang]

I'm wondering if we should just turn dev_err() into dev_dbg() instead and silence the error that way. Let userspace report the error if necessary.

@notro It's not easy to say it should be kernel module or usespace's work to info this. since the log just verboses a u32, confuses me at first look...we reparse it then we should classify it in the switch-case. When fb ioctls grow in 2d display driver, this will save us a lot time to say which is which.

[popcornmix]

As far as I can see FBUNSUPPORTED is a hack in fbturbo and isn't something handled by any other kernel fb driver. As such it could never be upstreamed and doesn't really belong in the kernel.

That leaves the options of fbturbo not doing this hack, or of the kernel downgrading an unknown ioctl into a more minor complaint, or of just living with the kernel messages.

@pelwell any thoughts?

[notro]

Looking at the fbdev core, I see that it doesn't log unsupported ioctl calls, it just returns -ENOTTY.
drivers/video/fbdev/core/fbmem.c:do_fb_ioctl()

static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
                        unsigned long arg)
{
<...>
        switch (cmd) {
<...>
        default:
                if (!lock_fb_info(info))
                        return -ENODEV;
                fb = info->fbops;
                if (fb->fb_ioctl)
                        ret = fb->fb_ioctl(info, cmd, arg);
                else
                        ret = -ENOTTY;
                unlock_fb_info(info);
        }
        return ret;
}

A quick look at some drivers shows examples of printing an error (omap2), returning zero even if it's unsupported (udlfb) and just returning an error (sh_mobile_lcdcfb).

[pelwell]

Supporting an ioctl that has been chosen specfically because it isn't supported does seem to be going against the spirit of the hack. We should just return an error - any decent code that cares will check it.

[pelwell]

Some drivers use ENOIOCTLCMD.

[notro]

Some drivers use ENOIOCTLCMD.

That would seem to be an obvious choice except for: These should never be seen by user programs.
include/linux/errno.h

/*
 * These should never be seen by user programs.  To return one of ERESTART*
 * codes, signal_pending() MUST be set.  Note that ptrace can observe these
 * at syscall exit tracing, but they will never be left for the debugged user
 * process to see.
 */
#define ERESTARTSYS     512
#define ERESTARTNOINTR  513
#define ERESTARTNOHAND  514     /* restart if no handler.. */
#define ENOIOCTLCMD     515     /* No ioctl command */

The correct return code for bad ioctl's seems to be ENOTTY:
http://lkml.iu.edu/hypermail/linux/kernel/0105.1/0734.html
https://lkml.org/lkml/2006/9/9/82

[pelwell]

That's fine with me.

[wuyuehang]

As far as I can see FBUNSUPPORTED is a hack in fbturbo and isn't something handled by any other kernel fb driver. As such it could never be upstreamed and doesn't really belong in the kernel.
That leaves the options of fbturbo not doing this hack, or of the kernel downgrading an unknown ioctl into a more minor complaint, or of just living with the kernel messages.

yes, once Xserver's 2d driver housekeeps its specified ioctls, we'd better sync with it. A better way to manager those ioctls is just bring in those specific ioctls inside bcm2708 fbdev's scope.

Looking at the fbdev core, I see that it doesn't log unsupported ioctl calls, it just returns -ENOTTY.
drivers/video/fbdev/core/fbmem.c:do_fb_ioctl()

ENOTTY does meet and come closely to what we need. But I do in favor of seeing the kernel message complaint when encounter unknown ioctls which is easier to catch our attention. What do you @notro @popcornmix @pelwell think of?
update patch bases on the talk. PTAL, thanks.

[popcornmix]

I'm still not keen on adding explicit support for FBUNSUPPORTED (which isn't a real kernel ioctl).

I think I could live with changing the current dev_err to a dev_dbg. I think that avoids the dmesg log message. It's not trivial to enable the message without a kernel rebuild, but hopefully if an error occurs it will be passed up the stack and reported from userland somewhere (maybe...)

So I think a two line patch that changes dev_err to dev_dbg and change the EINVAL to ENOTTY would be my preference.

[pelwell]

+1

[wuyuehang]

ok, I see, let the userspace worry the return value.
Based on all your opinions above, update patch and modify the commit message. Please spare
time to take a look @popcornmix @pelwell @notro , thanks:)

[pelwell]

Adding "Reviewed-by:" on someone else's behalf without explicit permission is slightly disingenuous, but this is a very simple change and we've already spent too long discussing it so I've merged anyway.

[wuyuehang]

Adding "Reviewed-by:" on someone else's behalf without explicit permission is slightly disingenuous, but this is a very simple change and we've already spent too long discussing it so I've merged anyway.

thank your for your reminder, will take care it next time. :p