The old rtc driver is getting in the way of some compat_ioctl simplification. Looking up the loongson64 git history, it seems that everyone uses the more modern but compatible RTC_CMOS driver anyway, so let's remove the special case for loongson64.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- arch/mips/Kconfig | 2 +- drivers/char/Kconfig | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig index 35511999156a..c695825d9377 100644 --- a/arch/mips/Kconfig +++ b/arch/mips/Kconfig @@ -75,7 +75,7 @@ config MIPS select MODULES_USE_ELF_RELA if MODULES && 64BIT select MODULES_USE_ELF_REL if MODULES select PERF_USE_VMALLOC - select RTC_LIB if !MACH_LOONGSON64 + select RTC_LIB select SYSCTL_EXCEPTION_TRACE select VIRT_TO_BUS
diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig index ce277ee0a28a..131b4c300050 100644 --- a/drivers/char/Kconfig +++ b/drivers/char/Kconfig @@ -268,7 +268,7 @@ if RTC_LIB=n
config RTC tristate "Enhanced Real Time Clock Support (legacy PC RTC driver)" - depends on ALPHA || (MIPS && MACH_LOONGSON64) + depends on ALPHA ---help--- If you say Y here and create a character special file /dev/rtc with major number 10 and minor number 135 using mknod ("man mknod"), you
We no longer need the rtc compat handling to be in common code, now that all drivers are either moved to the rtc-class framework, or (rarely) exist in drivers/char for architectures without compat mode (m68k, alpha and ia64, respectively).
I checked the list of ioctl commands in drivers, and the ones that are not already handled are all compatible, again with the one exception of m68k driver, which implements RTC_PLL_GET and RTC_PLL_SET, but has no compat mode.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/rtc/rtc-dev.c | 50 +++++++++++++++++++++++++++++++++++++++ fs/compat_ioctl.c | 54 ------------------------------------------- 2 files changed, 50 insertions(+), 54 deletions(-)
diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c index 43d962a9c210..50b03e175263 100644 --- a/drivers/rtc/rtc-dev.c +++ b/drivers/rtc/rtc-dev.c @@ -13,6 +13,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/compat.h> #include <linux/module.h> #include <linux/rtc.h> #include <linux/sched/signal.h> @@ -405,6 +406,52 @@ static long rtc_dev_ioctl(struct file *file, return err; }
+#ifdef CONFIG_COMPAT +#define RTC_IRQP_READ32 _IOR('p', 0x0b, compat_ulong_t) +#define RTC_IRQP_SET32 _IOW('p', 0x0c, compat_ulong_t) +#define RTC_EPOCH_READ32 _IOR('p', 0x0d, compat_ulong_t) +#define RTC_EPOCH_SET32 _IOW('p', 0x0e, compat_ulong_t) + +static long rtc_dev_compat_ioctl(struct file *file, unsigned cmd, unsigned long arg) +{ + unsigned long __user *valp; + compat_ulong_t __user *argp = compat_ptr(arg); + unsigned long val; + int ret; + + switch (cmd) { + /* pointer to 'long' needs translation. */ + case RTC_IRQP_READ32: + case RTC_EPOCH_READ32: { + valp = compat_alloc_user_space(sizeof(*valp)); + if (valp == NULL) + return -EFAULT; + ret = rtc_dev_ioctl(file, (cmd == RTC_IRQP_READ32) ? + RTC_IRQP_READ : RTC_EPOCH_READ, + (unsigned long)valp); + if (ret) + return ret; + if (get_user(val, valp) || put_user(val, argp)) + return -EFAULT; + return 0; + } + + /* arguments are compatible, command number is not */ + case RTC_IRQP_SET32: + return rtc_dev_ioctl(file, RTC_IRQP_SET, arg); + case RTC_EPOCH_SET32: + return rtc_dev_ioctl(file, RTC_EPOCH_SET, arg); + } + + /* + * all other rtc ioctls are compatible, or only + * exist on architectures without compat mode + */ + + return rtc_dev_ioctl(file, cmd, (unsigned long)argp); +} +#endif + static int rtc_dev_fasync(int fd, struct file *file, int on) { struct rtc_device *rtc = file->private_data; @@ -439,6 +486,9 @@ static const struct file_operations rtc_dev_fops = { .read = rtc_dev_read, .poll = rtc_dev_poll, .unlocked_ioctl = rtc_dev_ioctl, +#ifdef CONFIG_COMPAT + .compat_ioctl = rtc_dev_compat_ioctl, +#endif .open = rtc_dev_open, .release = rtc_dev_release, .fasync = rtc_dev_fasync, diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c index a9b00942e87d..a5c86a2fa8e6 100644 --- a/fs/compat_ioctl.c +++ b/fs/compat_ioctl.c @@ -46,7 +46,6 @@ #include <linux/raw.h> #include <linux/blkdev.h> #include <linux/elevator.h> -#include <linux/rtc.h> #include <linux/pci.h> #include <linux/serial.h> #include <linux/if_tun.h> @@ -623,37 +622,6 @@ static int serial_struct_ioctl(struct file *file, return err; }
-#define RTC_IRQP_READ32 _IOR('p', 0x0b, compat_ulong_t) -#define RTC_IRQP_SET32 _IOW('p', 0x0c, compat_ulong_t) -#define RTC_EPOCH_READ32 _IOR('p', 0x0d, compat_ulong_t) -#define RTC_EPOCH_SET32 _IOW('p', 0x0e, compat_ulong_t) - -static int rtc_ioctl(struct file *file, - unsigned cmd, void __user *argp) -{ - unsigned long __user *valp = compat_alloc_user_space(sizeof(*valp)); - int ret; - - if (valp == NULL) - return -EFAULT; - switch (cmd) { - case RTC_IRQP_READ32: - case RTC_EPOCH_READ32: - ret = do_ioctl(file, (cmd == RTC_IRQP_READ32) ? - RTC_IRQP_READ : RTC_EPOCH_READ, - (unsigned long)valp); - if (ret) - return ret; - return convert_in_user(valp, (unsigned int __user *)argp); - case RTC_IRQP_SET32: - return do_ioctl(file, RTC_IRQP_SET, (unsigned long)argp); - case RTC_EPOCH_SET32: - return do_ioctl(file, RTC_EPOCH_SET, (unsigned long)argp); - } - - return -ENOIOCTLCMD; -} - /* on ia32 l_start is on a 32-bit boundary */ #if defined(CONFIG_IA64) || defined(CONFIG_X86_64) struct space_resv_32 { @@ -806,21 +774,6 @@ COMPATIBLE_IOCTL(SCSI_IOCTL_GET_PCI) /* Big V (don't complain on serial console) */ IGNORE_IOCTL(VT_OPENQRY) IGNORE_IOCTL(VT_GETMODE) -/* Little p (/dev/rtc, /dev/envctrl, etc.) */ -COMPATIBLE_IOCTL(RTC_AIE_ON) -COMPATIBLE_IOCTL(RTC_AIE_OFF) -COMPATIBLE_IOCTL(RTC_UIE_ON) -COMPATIBLE_IOCTL(RTC_UIE_OFF) -COMPATIBLE_IOCTL(RTC_PIE_ON) -COMPATIBLE_IOCTL(RTC_PIE_OFF) -COMPATIBLE_IOCTL(RTC_WIE_ON) -COMPATIBLE_IOCTL(RTC_WIE_OFF) -COMPATIBLE_IOCTL(RTC_ALM_SET) -COMPATIBLE_IOCTL(RTC_ALM_READ) -COMPATIBLE_IOCTL(RTC_RD_TIME) -COMPATIBLE_IOCTL(RTC_SET_TIME) -COMPATIBLE_IOCTL(RTC_WKALM_SET) -COMPATIBLE_IOCTL(RTC_WKALM_RD) /* * These two are only for the sbus rtc driver, but * hwclock tries them on every rtc device first when @@ -1297,13 +1250,6 @@ static long do_ioctl_trans(unsigned int cmd, case TIOCGSERIAL: case TIOCSSERIAL: return serial_struct_ioctl(file, cmd, argp); - /* Not implemented in the native kernel */ - case RTC_IRQP_READ32: - case RTC_IRQP_SET32: - case RTC_EPOCH_READ32: - case RTC_EPOCH_SET32: - return rtc_ioctl(file, cmd, argp); - /* dvb */ case VIDEO_GET_EVENT: return do_video_get_event(file, cmd, argp);
On Mon, Aug 27, 2018 at 10:03:09PM +0200, Arnd Bergmann wrote:
+#ifdef CONFIG_COMPAT +#define RTC_IRQP_READ32 _IOR('p', 0x0b, compat_ulong_t) +#define RTC_IRQP_SET32 _IOW('p', 0x0c, compat_ulong_t) +#define RTC_EPOCH_READ32 _IOR('p', 0x0d, compat_ulong_t) +#define RTC_EPOCH_SET32 _IOW('p', 0x0e, compat_ulong_t)
+static long rtc_dev_compat_ioctl(struct file *file, unsigned cmd, unsigned long arg) +{
- unsigned long __user *valp;
- compat_ulong_t __user *argp = compat_ptr(arg);
- unsigned long val;
- int ret;
- switch (cmd) {
- /* pointer to 'long' needs translation. */
- case RTC_IRQP_READ32:
- case RTC_EPOCH_READ32: {
valp = compat_alloc_user_space(sizeof(*valp));if (valp == NULL)return -EFAULT;ret = rtc_dev_ioctl(file, (cmd == RTC_IRQP_READ32) ?RTC_IRQP_READ : RTC_EPOCH_READ,(unsigned long)valp);if (ret)return ret;if (get_user(val, valp) || put_user(val, argp))return -EFAULT;
No. With the side of Hell, No. This is bloody ridiculous - take a look at rtc_dev_ioctl(). Seriously. Relevant bits: struct rtc_device *rtc = file->private_data; const struct rtc_class_ops *ops = rtc->ops; struct rtc_time tm; struct rtc_wkalrm alarm; void __user *uarg = (void __user *) arg;
err = mutex_lock_interruptible(&rtc->ops_lock); if (err) return err;
err = put_user(rtc->irq_freq, (unsigned long __user *)uarg);
mutex_unlock(&rtc->ops_lock); return err;
That's RTC_IRQP_READ. IOW, all that song and dance starting with compat_alloc_user_space() is just a way to get to rtc->irq_freq value.
RTC_EPOCH_READ is in the bowels of individual implementations, but it's otherwise very similar; add a method returning the damn thing (e.g. rtc->ops->get_epoch(rtc)) and lift its call into rtc_dev_ioctl(), then this nonsense will go away as well.
return 0;- }
- /* arguments are compatible, command number is not */
- case RTC_IRQP_SET32:
return rtc_dev_ioctl(file, RTC_IRQP_SET, arg);
ITYM cmd = RTC_IRQP_SET; break;
- case RTC_EPOCH_SET32:
... and cmd = RTC_EPOCH_SET; break; here.
return rtc_dev_ioctl(file, RTC_EPOCH_SET, arg);- }
- /*
* all other rtc ioctls are compatible, or only* exist on architectures without compat mode*/- return rtc_dev_ioctl(file, cmd, (unsigned long)argp);
+}
compat_alloc_user_space() is usually papering over bogosities like that; if you are moving something out to individual drivers, it's always a red flag - most of the time it'll turn out to be pointless.
On Mon, Aug 27, 2018 at 10:21 PM Al Viro viro@zeniv.linux.org.uk wrote:
On Mon, Aug 27, 2018 at 10:03:09PM +0200, Arnd Bergmann wrote:
case RTC_IRQP_READ32:case RTC_EPOCH_READ32: {valp = compat_alloc_user_space(sizeof(*valp));if (valp == NULL)return -EFAULT;ret = rtc_dev_ioctl(file, (cmd == RTC_IRQP_READ32) ?RTC_IRQP_READ : RTC_EPOCH_READ,(unsigned long)valp);if (ret)return ret;if (get_user(val, valp) || put_user(val, argp))return -EFAULT;No. With the side of Hell, No. This is bloody ridiculous - take a lookat rtc_dev_ioctl(). Seriously. Relevant bits: struct rtc_device *rtc = file->private_data; const struct rtc_class_ops *ops = rtc->ops; struct rtc_time tm; struct rtc_wkalrm alarm; void __user *uarg = (void __user *) arg;
err = mutex_lock_interruptible(&rtc->ops_lock); if (err) return err; err = put_user(rtc->irq_freq, (unsigned long __user *)uarg); mutex_unlock(&rtc->ops_lock); return err;That's RTC_IRQP_READ. IOW, all that song and dance starting with compat_alloc_user_space() is just a way to get to rtc->irq_freq value.
RTC_EPOCH_READ is in the bowels of individual implementations, but it's otherwise very similar; add a method returning the damn thing (e.g. rtc->ops->get_epoch(rtc)) and lift its call into rtc_dev_ioctl(), then this nonsense will go away as well.
Okay. I was basically trying to do one step of moving the code in a more appropriate place without changing it much. The problem I see with a get_epoch() callback is that we don't actually want more drivers to implement it. At the moment there is only one (vr41xx), so how about just moving the compat epoch handling there and completely leaving it outside of the common rtc-dev implementation?
diff --git a/drivers/rtc/rtc-vr41xx.c b/drivers/rtc/rtc-vr41xx.c index 70f013e692b0..05f981f7cd21 100644 --- a/drivers/rtc/rtc-vr41xx.c +++ b/drivers/rtc/rtc-vr41xx.c @@ -17,6 +17,7 @@ * along with this program; if not, write to the Free Software * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ +#include <linux/compat.h> #include <linux/err.h> #include <linux/fs.h> #include <linux/init.h> @@ -195,6 +196,11 @@ static int vr41xx_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long switch (cmd) { case RTC_EPOCH_READ: return put_user(epoch, (unsigned long __user *)arg); +#ifdef CONFIG_COMPAT + case RTC_EPOCH_READ32: + return put_user(epoch, (unsigned int __user *)arg); + case RTC_EPOCH_SET32: +#endif case RTC_EPOCH_SET: /* Doesn't support before 1900 */
return 0;}/* arguments are compatible, command number is not */case RTC_IRQP_SET32:return rtc_dev_ioctl(file, RTC_IRQP_SET, arg);ITYM cmd = RTC_IRQP_SET; break;
case RTC_EPOCH_SET32:... and cmd = RTC_EPOCH_SET; break; here.
Just to clarify: do you mean I copied a bug from the old code, or are you objecting to the coding style?
return rtc_dev_ioctl(file, RTC_EPOCH_SET, arg);}/** all other rtc ioctls are compatible, or only* exist on architectures without compat mode*/return rtc_dev_ioctl(file, cmd, (unsigned long)argp);+}
compat_alloc_user_space() is usually papering over bogosities like that; if you are moving something out to individual drivers, it's always a red flag - most of the time it'll turn out to be pointless.
Absolutely agreed. Eliminating compat_alloc_user_space() was a bit lower on my priority list than killing off do_ioctl_trans() (which is already fairly low at the bottom), but I've adjusted that and will send an update.
I have a couple more patches for do_ioctl_trans() that started out as a side product of my y2038 patches, I'll make sure to do it the same way for any others. SG_GET_REQUEST_TABLE was the only other command for which I chose to keep compat_alloc_user_space() after my attempt to kill it ended up in way more complexity, but I can also revisit that, or maybe drop that patch until I get motivated to also tackle SG_IO.
Arnd
-
Al Viro -
Arnd Bergmann