[PATCH] UBI: add a ubi forced detach ioctl

[PATCH] UBI: add a ubi forced detach ioctl

[Luka Perkov]

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>
---
 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

Re: [PATCH] UBI: add a ubi forced detach ioctl

[Richard Weinberger]

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/



-- 
Thanks,
//richard

Re: [PATCH] UBI: add a ubi forced detach ioctl

[John Crispin]

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/
> 
> 
> 

Re: [PATCH] UBI: add a ubi forced detach ioctl

[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?
 
> 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

Re: [PATCH] UBI: add a ubi forced detach ioctl

[Richard Weinberger]

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

Re: [PATCH] UBI: add a ubi forced detach ioctl

[Luka Perkov]

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

Re: [PATCH] UBI: add a ubi forced detach ioctl

[Artem Bityutskiy]

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.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH] UBI: add a ubi forced detach ioctl

[John Crispin]

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