| Message ID | 1399850250-24616-1-git-send-email-luka@openwrt.org |
|---|---|
| State | Rejected |
| Headers | show |
On Mon, May 12, 2014 at 1:17 AM, Luka Perkov <luka@openwrt.org> wrote: > From: John Crispin <blogic@openwrt.org> > > Signed-off-by: John Crispin <blogic@openwrt.org> > Tested-by: Luka Perkov <luka@openwrt.org> > CC: Artem Bityutskiy <dedekind1@gmail.com> The changelog fails to describe why you need this new ioctl() and what problem this patch is solving. > --- > drivers/mtd/ubi/cdev.c | 7 +++++-- > include/uapi/mtd/ubi-user.h | 1 + > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c > index f54562a..dce1171 100644 > --- a/drivers/mtd/ubi/cdev.c > +++ b/drivers/mtd/ubi/cdev.c > @@ -970,7 +970,7 @@ static long ubi_cdev_ioctl(struct file *file, unsigned int cmd, > static long ctrl_cdev_ioctl(struct file *file, unsigned int cmd, > unsigned long arg) > { > - int err = 0; > + int err = 0, force = 0; > void __user *argp = (void __user *)arg; > > if (!capable(CAP_SYS_RESOURCE)) > @@ -1020,6 +1020,9 @@ static long ctrl_cdev_ioctl(struct file *file, unsigned int cmd, > } > > /* Detach an MTD device command */ > + case UBI_IOCFDET: > + force = 1; > + /* fallthrough */ > case UBI_IOCDET: > { > int ubi_num; > @@ -1032,7 +1035,7 @@ static long ctrl_cdev_ioctl(struct file *file, unsigned int cmd, > } > > mutex_lock(&ubi_devices_mutex); > - err = ubi_detach_mtd_dev(ubi_num, 0); > + err = ubi_detach_mtd_dev(ubi_num, force); > mutex_unlock(&ubi_devices_mutex); > break; > } > diff --git a/include/uapi/mtd/ubi-user.h b/include/uapi/mtd/ubi-user.h > index 1927b0d..7600e18 100644 > --- a/include/uapi/mtd/ubi-user.h > +++ b/include/uapi/mtd/ubi-user.h > @@ -178,6 +178,7 @@ > #define UBI_IOCATT _IOW(UBI_CTRL_IOC_MAGIC, 64, struct ubi_attach_req) > /* Detach an MTD device */ > #define UBI_IOCDET _IOW(UBI_CTRL_IOC_MAGIC, 65, __s32) > +#define UBI_IOCFDET _IOW(UBI_CTRL_IOC_MAGIC, 66, __s32) > > /* ioctl commands of UBI volume character devices */ > > -- > 1.9.2 > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hi Luka, This is a ugly temporary patch that we carry around in openwrt until we have a real fix. why are you trying to upstream this ? Additionally the patch was written by Daniel and not me so the SoB is wrong. John On 12/05/2014 09:14, Richard Weinberger wrote: > On Mon, May 12, 2014 at 1:17 AM, Luka Perkov <luka@openwrt.org> > wrote: >> From: John Crispin <blogic@openwrt.org> >> >> Signed-off-by: John Crispin <blogic@openwrt.org> Tested-by: Luka >> Perkov <luka@openwrt.org> CC: Artem Bityutskiy >> <dedekind1@gmail.com> > > The changelog fails to describe why you need this new ioctl() and > what problem this patch is solving. > >> --- drivers/mtd/ubi/cdev.c | 7 +++++-- >> include/uapi/mtd/ubi-user.h | 1 + 2 files changed, 6 >> insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c >> index f54562a..dce1171 100644 --- a/drivers/mtd/ubi/cdev.c +++ >> b/drivers/mtd/ubi/cdev.c @@ -970,7 +970,7 @@ static long >> ubi_cdev_ioctl(struct file *file, unsigned int cmd, static long >> ctrl_cdev_ioctl(struct file *file, unsigned int cmd, unsigned >> long arg) { - int err = 0; + int err = 0, force = 0; >> void __user *argp = (void __user *)arg; >> >> if (!capable(CAP_SYS_RESOURCE)) @@ -1020,6 +1020,9 @@ static >> long ctrl_cdev_ioctl(struct file *file, unsigned int cmd, } >> >> /* Detach an MTD device command */ + case UBI_IOCFDET: + >> force = 1; + /* fallthrough */ case UBI_IOCDET: { >> int ubi_num; @@ -1032,7 +1035,7 @@ static long >> ctrl_cdev_ioctl(struct file *file, unsigned int cmd, } >> >> mutex_lock(&ubi_devices_mutex); - err = >> ubi_detach_mtd_dev(ubi_num, 0); + err = >> ubi_detach_mtd_dev(ubi_num, force); >> mutex_unlock(&ubi_devices_mutex); break; } diff --git >> a/include/uapi/mtd/ubi-user.h b/include/uapi/mtd/ubi-user.h >> index 1927b0d..7600e18 100644 --- a/include/uapi/mtd/ubi-user.h >> +++ b/include/uapi/mtd/ubi-user.h @@ -178,6 +178,7 @@ #define >> UBI_IOCATT _IOW(UBI_CTRL_IOC_MAGIC, 64, struct ubi_attach_req) >> /* Detach an MTD device */ #define UBI_IOCDET >> _IOW(UBI_CTRL_IOC_MAGIC, 65, __s32) +#define UBI_IOCFDET >> _IOW(UBI_CTRL_IOC_MAGIC, 66, __s32) >> >> /* ioctl commands of UBI volume character devices */ >> >> -- 1.9.2 >> >> ______________________________________________________ Linux MTD >> discussion mailing list >> http://lists.infradead.org/mailman/listinfo/linux-mtd/ > > >
On Mon, May 12, 2014 at 11:21:22AM +0200, John Crispin wrote: > This is a ugly temporary patch that we carry around in openwrt until we > have a real fix. why are you trying to upstream this ? I didn't think this is a hack, other file systems can force umount as well. What are you proposing as a real fix? > Additionally the patch was written by Daniel and not me so the SoB is > wrong. I took it from OpenWrt and it was commited there with your SoB and Daniel was not mentioned there. > On 12/05/2014 09:14, Richard Weinberger wrote: > > On Mon, May 12, 2014 at 1:17 AM, Luka Perkov <luka@openwrt.org> > > wrote: > >> From: John Crispin <blogic@openwrt.org> > >> > >> Signed-off-by: John Crispin <blogic@openwrt.org> Tested-by: Luka > >> Perkov <luka@openwrt.org> CC: Artem Bityutskiy > >> <dedekind1@gmail.com> > > > > The changelog fails to describe why you need this new ioctl() and > > what problem this patch is solving. When running ubi rootfs upgrade on nand based OpenWrt device after pivot_root init process is still "hooked" on the old file system. Thus, the old file system can not be umounted. If the filesystem is mounted it can not be upgraded with using for example ubiupdatevol or removed with ubirmvol. Forcing umount would allow to run the before mentioned commands. Luka
Am 12.05.2014 12:44, schrieb Luka Perkov: > On Mon, May 12, 2014 at 11:21:22AM +0200, John Crispin wrote: >> This is a ugly temporary patch that we carry around in openwrt until we >> have a real fix. why are you trying to upstream this ? > > I didn't think this is a hack, other file systems can force umount as > well. What are you proposing as a real fix? You're messing with UBI, not UBIFS. :-) So, what problem is your patch solving and why do we need it upstream? Thanks, //richard
On Mon, May 12, 2014 at 03:47:17PM +0200, Richard Weinberger wrote: > Am 12.05.2014 12:44, schrieb Luka Perkov: > > On Mon, May 12, 2014 at 11:21:22AM +0200, John Crispin wrote: > >> This is a ugly temporary patch that we carry around in openwrt until we > >> have a real fix. why are you trying to upstream this ? > > > > I didn't think this is a hack, other file systems can force umount as > > well. What are you proposing as a real fix? > > You're messing with UBI, not UBIFS. :-) True. > So, what problem is your patch solving and why do we need it upstream? You can drop it. Thanks! Luka
On Mon, 2014-05-12 at 12:44 +0200, Luka Perkov wrote: > When running ubi rootfs upgrade on nand based OpenWrt device after > pivot_root init process is still "hooked" on the old file system. Thus, > the old file system can not be umounted. If the filesystem is mounted it > can not be upgraded with using for example ubiupdatevol or removed with > ubirmvol. Forcing umount would allow to run the before mentioned > commands. Generally, a change like this may be fine, but I'd like to see more detailed description and analysis, which points to how other kernel subsystems deal with this.
On 13/05/2014 11:03, Artem Bityutskiy wrote: > On Mon, 2014-05-12 at 12:44 +0200, Luka Perkov wrote: >> When running ubi rootfs upgrade on nand based OpenWrt device >> after pivot_root init process is still "hooked" on the old file >> system. Thus, the old file system can not be umounted. If the >> filesystem is mounted it can not be upgraded with using for >> example ubiupdatevol or removed with ubirmvol. Forcing umount >> would allow to run the before mentioned commands. > > Generally, a change like this may be fine, but I'd like to see > more detailed description and analysis, which points to how other > kernel subsystems deal with this. > Hi Artem, agreed, this patch is an interim hack we did to make a new piece of code work that helps us to sysupgrade the ubi when a new openwrt release is flashed. this tool is far from complete and is still missing a lot of details. once we are done and know if this patch makes sense in its current form we will let you know and resend this or an alternate patch with a proper description. Would that be ok with you ? John
diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c index f54562a..dce1171 100644 --- a/drivers/mtd/ubi/cdev.c +++ b/drivers/mtd/ubi/cdev.c @@ -970,7 +970,7 @@ static long ubi_cdev_ioctl(struct file *file, unsigned int cmd, static long ctrl_cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { - int err = 0; + int err = 0, force = 0; void __user *argp = (void __user *)arg; if (!capable(CAP_SYS_RESOURCE)) @@ -1020,6 +1020,9 @@ static long ctrl_cdev_ioctl(struct file *file, unsigned int cmd, } /* Detach an MTD device command */ + case UBI_IOCFDET: + force = 1; + /* fallthrough */ case UBI_IOCDET: { int ubi_num; @@ -1032,7 +1035,7 @@ static long ctrl_cdev_ioctl(struct file *file, unsigned int cmd, } mutex_lock(&ubi_devices_mutex); - err = ubi_detach_mtd_dev(ubi_num, 0); + err = ubi_detach_mtd_dev(ubi_num, force); mutex_unlock(&ubi_devices_mutex); break; } diff --git a/include/uapi/mtd/ubi-user.h b/include/uapi/mtd/ubi-user.h index 1927b0d..7600e18 100644 --- a/include/uapi/mtd/ubi-user.h +++ b/include/uapi/mtd/ubi-user.h @@ -178,6 +178,7 @@ #define UBI_IOCATT _IOW(UBI_CTRL_IOC_MAGIC, 64, struct ubi_attach_req) /* Detach an MTD device */ #define UBI_IOCDET _IOW(UBI_CTRL_IOC_MAGIC, 65, __s32) +#define UBI_IOCFDET _IOW(UBI_CTRL_IOC_MAGIC, 66, __s32) /* ioctl commands of UBI volume character devices */