Re: [PATCH] erofs: Use common kernel logging style

2019-08-18 Thread Gao Xiang
Hi Joe,

On Sun, Aug 18, 2019 at 10:47:17PM -0700, Joe Perches wrote:
> On Mon, 2019-08-19 at 13:52 +0800, Gao Xiang wrote:
> > Hi Joe,
> 
> Hello.
> 
> > On Sun, Aug 18, 2019 at 10:28:41PM -0700, Joe Perches wrote:
> > > Rename errln, infoln, and debugln to the typical pr_ uses
> > > to the typical kernel styles of pr_
> > 
> > How about using erofs_err / ... to instead that?
> 
>   I've no opinion.
> It seems most fs/*/* filesystems actually do use pr_
> sed works well if you want that.

Sorry, I mainly refer to ext4, ext2, xfs and f2fs...
I didn't notice the other filesystems, you are right.

Okay, I have no opinion as well (maybe we could turn back later to
introduce sb parameter...)

> 
> >  - I can hardly see directly use pr_ for those filesystems in fs/...
> 
> just fyi:
> 
> There was this one existing pr_ use in erofs

That is just because the following piece of code was taken from fs/mpage.c,
I tend to replace it with iomap in the later version after tail-end packing 
inline is done.

Thanks,
Gao Xiang

> 
> drivers/staging/erofs/data.c:366:   pr_err("%s, 
> readahead error at page %lu of nid %llu\n",
> drivers/staging/erofs/data.c-367-  
> __func__, page->index,
> drivers/staging/erofs/data.c-368-  
> EROFS_V(mapping->host)->nid);
> 
> 
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] erofs: Use common kernel logging style

2019-08-18 Thread Joe Perches
On Mon, 2019-08-19 at 13:52 +0800, Gao Xiang wrote:
> Hi Joe,

Hello.

> On Sun, Aug 18, 2019 at 10:28:41PM -0700, Joe Perches wrote:
> > Rename errln, infoln, and debugln to the typical pr_ uses
> > to the typical kernel styles of pr_
> 
> How about using erofs_err / ... to instead that?

  I've no opinion.
It seems most fs/*/* filesystems actually do use pr_
sed works well if you want that.

>  - I can hardly see directly use pr_ for those filesystems in fs/...

just fyi:

There was this one existing pr_ use in erofs

drivers/staging/erofs/data.c:366:   pr_err("%s, 
readahead error at page %lu of nid %llu\n",
drivers/staging/erofs/data.c-367-  
__func__, page->index,
drivers/staging/erofs/data.c-368-  
EROFS_V(mapping->host)->nid);



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [RFC PATCH 00/15] acrn: add the ACRN driver module

2019-08-18 Thread Zhao, Yakui



On 2019年08月19日 13:25, Greg KH wrote:

On Mon, Aug 19, 2019 at 09:44:25AM +0800, Zhao, Yakui wrote:



On 2019年08月16日 14:39, Borislav Petkov wrote:

On Fri, Aug 16, 2019 at 10:25:41AM +0800, Zhao Yakui wrote:

The first three patches are the changes under x86/acrn, which adds the
required APIs for the driver and reports the X2APIC caps.
The remaining patches add the ACRN driver module, which accepts the ioctl
from user-space and then communicate with the low-level ACRN hypervisor
by using hypercall.


I have a problem with that: you're adding interfaces to arch/x86/ and
its users go into staging. Why? Why not directly put the driver where
it belongs, clean it up properly and submit it like everything else is
submitted?


Thanks for your reply and the concern.

After taking a look at several driver examples(gma500, android), it seems
that they are firstly added into drivers/staging/XXX and then moved to
drivers/XXX after the driver becomes mature.
So we refer to this method to upstream ACRN driver part.


Those two examples are probably the worst examples to ever look at :)

The code quality of those submissions was horrible, gma500 took a very
long time to clean up and there are parts of the android code that are
still in staging to this day.


If the new driver can also be added by skipping the staging approach,
we will refine it and then submit it in normal process.


That is the normal process, staging should not be needed at all for any
code.  It is a fall-back for when the company involved has no idea of
how to upstream their code, which should NOT be the case here.


Thanks for your explanation.

OK. We will submit it in normal process.



thanks,

greg k-h


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


[PATCH] erofs: Use common kernel logging style

2019-08-18 Thread Joe Perches
Rename errln, infoln, and debugln to the typical pr_ uses
to the typical kernel styles of pr_

Miscellanea:

o Add newline terminations to the uses
o Use "%s: ...", __func__ and not the atypical "%s, ...", __func__
o Trivial grammar changes in output logging
o Delete the now unused macros

Signed-off-by: Joe Perches 
---
 drivers/staging/erofs/data.c |  6 ++--
 drivers/staging/erofs/decompressor.c |  6 ++--
 drivers/staging/erofs/dir.c  |  8 +++---
 drivers/staging/erofs/inode.c| 16 +--
 drivers/staging/erofs/internal.h |  8 ++
 drivers/staging/erofs/namei.c|  4 +--
 drivers/staging/erofs/super.c| 54 +++-
 drivers/staging/erofs/xattr.c|  4 +--
 drivers/staging/erofs/zdata.c| 12 
 drivers/staging/erofs/zdata.h|  2 +-
 drivers/staging/erofs/zmap.c | 26 -
 11 files changed, 73 insertions(+), 73 deletions(-)

diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
index 4cdb743c8b8d..677d95e8fdeb 100644
--- a/drivers/staging/erofs/data.c
+++ b/drivers/staging/erofs/data.c
@@ -152,8 +152,8 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
 
map->m_flags |= EROFS_MAP_META;
} else {
-   errln("internal error @ nid: %llu (size %llu), m_la 0x%llx",
- vi->nid, inode->i_size, map->m_la);
+   pr_err("internal error @ nid: %llu (size %llu), m_la 0x%llx\n",
+  vi->nid, inode->i_size, map->m_la);
DBG_BUGON(1);
err = -EIO;
goto err_out;
@@ -363,7 +363,7 @@ static int erofs_raw_access_readpages(struct file *filp,
 
/* all the page errors are ignored when readahead */
if (IS_ERR(bio)) {
-   pr_err("%s, readahead error at page %lu of nid 
%llu\n",
+   pr_err("%s: readahead error at page %lu of nid 
%llu\n",
   __func__, page->index,
   EROFS_V(mapping->host)->nid);
 
diff --git a/drivers/staging/erofs/decompressor.c 
b/drivers/staging/erofs/decompressor.c
index 5361a2bbedb6..24d450ce66c1 100644
--- a/drivers/staging/erofs/decompressor.c
+++ b/drivers/staging/erofs/decompressor.c
@@ -166,9 +166,9 @@ static int lz4_decompress(struct z_erofs_decompress_req 
*rq, u8 *out)
  inlen, rq->outputsize,
  rq->outputsize);
if (ret < 0) {
-   errln("%s, failed to decompress, in[%p, %u, %u] out[%p, %u]",
- __func__, src + inputmargin, inlen, inputmargin,
- out, rq->outputsize);
+   pr_err("%s: failed to decompress, in[%p, %u, %u] out[%p, %u]\n",
+  __func__, src + inputmargin, inlen, inputmargin,
+  out, rq->outputsize);
WARN_ON(1);
print_hex_dump(KERN_DEBUG, "[ in]: ", DUMP_PREFIX_OFFSET,
   16, 1, src + inputmargin, inlen, true);
diff --git a/drivers/staging/erofs/dir.c b/drivers/staging/erofs/dir.c
index 2fbfc4935077..526c7b5dd4db 100644
--- a/drivers/staging/erofs/dir.c
+++ b/drivers/staging/erofs/dir.c
@@ -29,8 +29,8 @@ static void debug_one_dentry(unsigned char d_type, const char 
*de_name,
memcpy(dbg_namebuf, de_name, de_namelen);
dbg_namebuf[de_namelen] = '\0';
 
-   debugln("found dirent %s de_len %u d_type %d", dbg_namebuf,
-   de_namelen, d_type);
+   pr_debug("found dirent %s de_len %u d_type %d\n",
+dbg_namebuf, de_namelen, d_type);
 #endif
 }
 
@@ -104,8 +104,8 @@ static int erofs_readdir(struct file *f, struct dir_context 
*ctx)
 
if (unlikely(nameoff < sizeof(struct erofs_dirent) ||
 nameoff >= PAGE_SIZE)) {
-   errln("%s, invalid de[0].nameoff %u",
- __func__, nameoff);
+   pr_err("%s: invalid de[0].nameoff %u\n",
+  __func__, nameoff);
 
err = -EIO;
goto skip_this;
diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c
index 286729143365..7b91f3baf8d4 100644
--- a/drivers/staging/erofs/inode.c
+++ b/drivers/staging/erofs/inode.c
@@ -21,8 +21,8 @@ static int read_inode(struct inode *inode, void *data)
vi->datamode = __inode_data_mapping(advise);
 
if (unlikely(vi->datamode >= EROFS_INODE_LAYOUT_MAX)) {
-   errln("unsupported data mapping %u of nid %llu",
- vi->datamode, vi->nid);
+   pr_err("unsupported data mapping %u of nid %llu\n",
+  vi->datamode, vi->nid);
DBG_BUGON(1);
return -EIO;
}
@@ -92,8 +92,8 @@ static int 

Re: [RFC PATCH 08/15] drivers/acrn: add VM memory management for ACRN char device

2019-08-18 Thread Zhao, Yakui



On 2019年08月16日 20:58, Dan Carpenter wrote:

On Fri, Aug 16, 2019 at 10:25:49AM +0800, Zhao Yakui wrote:

+int hugepage_map_guest(struct acrn_vm *vm, struct vm_memmap *memmap)
+{
+   struct page *page = NULL, *regions_buf_pg = NULL;
+   unsigned long len, guest_gpa, vma;
+   struct vm_memory_region *region_array;
+   struct set_regions *regions;
+   int max_size = PAGE_SIZE / sizeof(struct vm_memory_region);
+   int ret;
+
+   if (!vm || !memmap)
+   return -EINVAL;
+
+   len = memmap->len;
+   vma = memmap->vma_base;
+   guest_gpa = memmap->gpa;
+
+   /* prepare set_memory_regions info */
+   regions_buf_pg = alloc_page(GFP_KERNEL);
+   if (!regions_buf_pg)
+   return -ENOMEM;
+
+   regions = kzalloc(sizeof(*regions), GFP_KERNEL);
+   if (!regions) {
+   __free_page(regions_buf_pg);
+   return -ENOMEM;


It's better to do a goto err_free_regions_buf here.  More comments
below.


+   }
+   regions->mr_num = 0;
+   regions->vmid = vm->vmid;
+   regions->regions_gpa = page_to_phys(regions_buf_pg);
+   region_array = page_to_virt(regions_buf_pg);
+
+   while (len > 0) {
+   unsigned long vm0_gpa, pagesize;
+
+   ret = get_user_pages_fast(vma, 1, 1, );
+   if (unlikely(ret != 1) || (!page)) {
+   pr_err("failed to pin huge page!\n");
+   ret = -ENOMEM;
+   goto err;


goto err is a red flag.  It's better if error labels do one specific
named thing like:

err_regions:
kfree(regions);
err_free_regions_buf:
__free_page(regions_buf_pg);

We should unwind in the opposite/mirror order from how things were
allocated.  Then we can remove the if statements in the error handling.


Thanks for the review.

Will follow your suggestion to unwind the error handling.



In this situation, say the user triggers an -EFAULT in
get_user_pages_fast() in the second iteration through the loop.  That
means that "page" is the non-NULL page from the previous iteration.  We
have already added it to add_guest_map().  But now we're freeing it
without removing it from the map so probably it leads to a use after
free.

The best way to write the error handling in a loop like this is to
clean up the partial iteration that has succeed (nothing here), and then
unwind all the successful iterations at the bottom of the function.
"goto unwind_loop;"



In theory we should cleanup the previous success iteration if it 
encounters one error in the current iteration.

But it will be quite complex to cleanup up the previous iteration.
call the set_memory_regions for MR_DEL op.
call the remove_guest_map for the added hash item
call the put_page for returned page in get_user_pages_fast.

In fact as this driver is mainly used for embedded IOT usage, it doesn't 
handle the complex cleanup when such error is encountered. Instead the 
clean up is handled in free_guest_vm.



+   }
+
+   vm0_gpa = page_to_phys(page);
+   pagesize = PAGE_SIZE << compound_order(page);
+
+   ret = add_guest_map(vm, vm0_gpa, guest_gpa, pagesize);
+   if (ret < 0) {
+   pr_err("failed to add memseg for huge page!\n");
+   goto err;


So then here, it would be:

pr_err("failed to add memseg for huge page!\n");
put_page(page);
goto unwind_loop;

regards,
dan carpenter


+   }
+
+   /* fill each memory region into region_array */
+   region_array[regions->mr_num].type = MR_ADD;
+   region_array[regions->mr_num].gpa = guest_gpa;
+   region_array[regions->mr_num].vm0_gpa = vm0_gpa;
+   region_array[regions->mr_num].size = pagesize;
+   region_array[regions->mr_num].prot =
+   (MEM_TYPE_WB & MEM_TYPE_MASK) |
+   (memmap->prot & MEM_ACCESS_RIGHT_MASK);
+   regions->mr_num++;
+   if (regions->mr_num == max_size) {
+   pr_debug("region buffer full, set & renew regions!\n");
+   ret = set_memory_regions(regions);
+   if (ret < 0) {
+   pr_err("failed to set regions,ret=%d!\n", ret);
+   goto err;
+   }
+   regions->mr_num = 0;
+   }
+
+   len -= pagesize;
+   vma += pagesize;
+   guest_gpa += pagesize;
+   }
+
+   ret = set_memory_regions(regions);
+   if (ret < 0) {
+   pr_err("failed to set regions, ret=%d!\n", ret);
+   goto err;
+   }
+
+   __free_page(regions_buf_pg);
+   kfree(regions);
+
+   return 0;
+err:
+   if (regions_buf_pg)
+   

Re: [PATCH] erofs: Use common kernel logging style

2019-08-18 Thread Gao Xiang
Hi Joe,

On Sun, Aug 18, 2019 at 10:28:41PM -0700, Joe Perches wrote:
> Rename errln, infoln, and debugln to the typical pr_ uses
> to the typical kernel styles of pr_

How about using erofs_err / ... to instead that?
 - I can hardly see directly use pr_ for those filesystems in fs/...

 - maybe we will introduce super_block to print more information
   about the specific filesystem...

> 
> Miscellanea:
> 
> o Add newline terminations to the uses
> o Use "%s: ...", __func__ and not the atypical "%s, ...", __func__

Agreed.

Thanks,
Gao Xiang

> o Trivial grammar changes in output logging
> o Delete the now unused macros
> 
> Signed-off-by: Joe Perches 
> ---
>  drivers/staging/erofs/data.c |  6 ++--
>  drivers/staging/erofs/decompressor.c |  6 ++--
>  drivers/staging/erofs/dir.c  |  8 +++---
>  drivers/staging/erofs/inode.c| 16 +--
>  drivers/staging/erofs/internal.h |  8 ++
>  drivers/staging/erofs/namei.c|  4 +--
>  drivers/staging/erofs/super.c| 54 
> +++-
>  drivers/staging/erofs/xattr.c|  4 +--
>  drivers/staging/erofs/zdata.c| 12 
>  drivers/staging/erofs/zdata.h|  2 +-
>  drivers/staging/erofs/zmap.c | 26 -
>  11 files changed, 73 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
> index 4cdb743c8b8d..677d95e8fdeb 100644
> --- a/drivers/staging/erofs/data.c
> +++ b/drivers/staging/erofs/data.c
> @@ -152,8 +152,8 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
>  
>   map->m_flags |= EROFS_MAP_META;
>   } else {
> - errln("internal error @ nid: %llu (size %llu), m_la 0x%llx",
> -   vi->nid, inode->i_size, map->m_la);
> + pr_err("internal error @ nid: %llu (size %llu), m_la 0x%llx\n",
> +vi->nid, inode->i_size, map->m_la);
>   DBG_BUGON(1);
>   err = -EIO;
>   goto err_out;
> @@ -363,7 +363,7 @@ static int erofs_raw_access_readpages(struct file *filp,
>  
>   /* all the page errors are ignored when readahead */
>   if (IS_ERR(bio)) {
> - pr_err("%s, readahead error at page %lu of nid 
> %llu\n",
> + pr_err("%s: readahead error at page %lu of nid 
> %llu\n",
>  __func__, page->index,
>  EROFS_V(mapping->host)->nid);
>  
> diff --git a/drivers/staging/erofs/decompressor.c 
> b/drivers/staging/erofs/decompressor.c
> index 5361a2bbedb6..24d450ce66c1 100644
> --- a/drivers/staging/erofs/decompressor.c
> +++ b/drivers/staging/erofs/decompressor.c
> @@ -166,9 +166,9 @@ static int lz4_decompress(struct z_erofs_decompress_req 
> *rq, u8 *out)
> inlen, rq->outputsize,
> rq->outputsize);
>   if (ret < 0) {
> - errln("%s, failed to decompress, in[%p, %u, %u] out[%p, %u]",
> -   __func__, src + inputmargin, inlen, inputmargin,
> -   out, rq->outputsize);
> + pr_err("%s: failed to decompress, in[%p, %u, %u] out[%p, %u]\n",
> +__func__, src + inputmargin, inlen, inputmargin,
> +out, rq->outputsize);
>   WARN_ON(1);
>   print_hex_dump(KERN_DEBUG, "[ in]: ", DUMP_PREFIX_OFFSET,
>  16, 1, src + inputmargin, inlen, true);
> diff --git a/drivers/staging/erofs/dir.c b/drivers/staging/erofs/dir.c
> index 2fbfc4935077..526c7b5dd4db 100644
> --- a/drivers/staging/erofs/dir.c
> +++ b/drivers/staging/erofs/dir.c
> @@ -29,8 +29,8 @@ static void debug_one_dentry(unsigned char d_type, const 
> char *de_name,
>   memcpy(dbg_namebuf, de_name, de_namelen);
>   dbg_namebuf[de_namelen] = '\0';
>  
> - debugln("found dirent %s de_len %u d_type %d", dbg_namebuf,
> - de_namelen, d_type);
> + pr_debug("found dirent %s de_len %u d_type %d\n",
> +  dbg_namebuf, de_namelen, d_type);
>  #endif
>  }
>  
> @@ -104,8 +104,8 @@ static int erofs_readdir(struct file *f, struct 
> dir_context *ctx)
>  
>   if (unlikely(nameoff < sizeof(struct erofs_dirent) ||
>nameoff >= PAGE_SIZE)) {
> - errln("%s, invalid de[0].nameoff %u",
> -   __func__, nameoff);
> + pr_err("%s: invalid de[0].nameoff %u\n",
> +__func__, nameoff);
>  
>   err = -EIO;
>   goto skip_this;
> diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c
> index 286729143365..7b91f3baf8d4 100644
> --- a/drivers/staging/erofs/inode.c
> +++ b/drivers/staging/erofs/inode.c
> @@ -21,8 +21,8 @@ static int read_inode(struct inode *inode, void *data)
>   

Re: [RFC PATCH 04/15] drivers/acrn: add the basic framework of acrn char device driver

2019-08-18 Thread Greg KH
On Mon, Aug 19, 2019 at 12:02:33PM +0800, Zhao, Yakui wrote:
> 
> 
> On 2019年08月16日 15:05, Greg KH wrote:
> > On Fri, Aug 16, 2019 at 10:25:45AM +0800, Zhao Yakui wrote:
> > > ACRN hypervisor service module is the important middle layer that allows
> > > the Linux kernel to communicate with the ACRN hypervisor. It includes
> > > the management of virtualized CPU/memory/device/interrupt for other ACRN
> > > guest. The user-space applications can use the provided ACRN ioctls to
> > > interact with ACRN hypervisor through different hypercalls.
> > > 
> > > Add one basic framework firstly and the following patches will
> > > add the corresponding implementations, which includes the management of
> > > virtualized CPU/memory/interrupt and the emulation of MMIO/IO/PCI access.
> > > The device file of /dev/acrn_hsm can be accessed in user-space to
> > > communicate with ACRN module.
> > > 
> > > Co-developed-by: Jason Chen CJ 
> > > Signed-off-by: Jason Chen CJ 
> > > Co-developed-by: Jack Ren 
> > > Signed-off-by: Jack Ren 
> > > Co-developed-by: Mingqiang Chi 
> > > Signed-off-by: Mingqiang Chi 
> > > Co-developed-by: Liu Shuo 
> > > Signed-off-by: Liu Shuo 
> > > Signed-off-by: Zhao Yakui 
> > > ---
> > >   drivers/staging/Kconfig |   2 +
> > 
> > Also, your subject line for all of these patches are wrong, it is not
> > drivers/acrn :(
> 
> Thanks for the pointing out it.
> 
> It will be fixed.
> 
> > 
> > And you forgot to cc: the staging maintainer :(
> 
> Do you mean that the maintainer of staging subsystem is also added in the
> patch commit log?

Did you not run scripts/get_maintainer.pl on your patches to determine
who to send patches to?  Always do that.

> > As I have said with NUMEROUS Intel patches in the past, I now refuse to
> > take patches from you all WITHOUT having it signed-off-by someone from
> > the Intel "OTC" group (or whatever the Intel Linux group is called these
> > days).  They are a resource you can not ignore, and if you do, you just
> > end up making the rest of the kernel community grumpy by having us do
> > their work for them :(
> > 
> > Please work with them.
> 
> OK. I will work with some peoples in OTC group to prepare the better ACRN
> driver.

Thank you.

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [RFC PATCH 00/15] acrn: add the ACRN driver module

2019-08-18 Thread Greg KH
On Mon, Aug 19, 2019 at 10:39:32AM +0800, Zhao, Yakui wrote:
> 
> 
> On 2019年08月16日 15:03, Greg KH wrote:
> > On Fri, Aug 16, 2019 at 08:39:25AM +0200, Borislav Petkov wrote:
> > > On Fri, Aug 16, 2019 at 10:25:41AM +0800, Zhao Yakui wrote:
> > > > The first three patches are the changes under x86/acrn, which adds the
> > > > required APIs for the driver and reports the X2APIC caps.
> > > > The remaining patches add the ACRN driver module, which accepts the 
> > > > ioctl
> > > > from user-space and then communicate with the low-level ACRN hypervisor
> > > > by using hypercall.
> > > 
> > > I have a problem with that: you're adding interfaces to arch/x86/ and
> > > its users go into staging. Why? Why not directly put the driver where
> > > it belongs, clean it up properly and submit it like everything else is
> > > submitted?
> > > 
> > > I don't want to have stuff in arch/x86/ which is used solely by code in
> > > staging and the latter is lingering there indefinitely because no one is
> > > cleaning it up...
> > 
> > I agree, stuff in drivers/staging/ must be self-contained, with no
> > changes outside of the code's subdirectory needed in order for it to
> > work.  That way it is trivial for us to delete it when it never gets
> > cleaned up :)
> 
> Thanks for pointing out the rule of drivers/staging.
> The acrn staging driver is one self-contained driver. But it has some
> dependency on arch/x86/acrn and need to call the APIs in arch/x86/acrn.

Then it should not be in drivers/staging/  Please work to get this
accepted "normally".

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [RFC PATCH 00/15] acrn: add the ACRN driver module

2019-08-18 Thread Greg KH
On Mon, Aug 19, 2019 at 09:44:25AM +0800, Zhao, Yakui wrote:
> 
> 
> On 2019年08月16日 14:39, Borislav Petkov wrote:
> > On Fri, Aug 16, 2019 at 10:25:41AM +0800, Zhao Yakui wrote:
> > > The first three patches are the changes under x86/acrn, which adds the
> > > required APIs for the driver and reports the X2APIC caps.
> > > The remaining patches add the ACRN driver module, which accepts the ioctl
> > > from user-space and then communicate with the low-level ACRN hypervisor
> > > by using hypercall.
> > 
> > I have a problem with that: you're adding interfaces to arch/x86/ and
> > its users go into staging. Why? Why not directly put the driver where
> > it belongs, clean it up properly and submit it like everything else is
> > submitted?
> 
> Thanks for your reply and the concern.
> 
> After taking a look at several driver examples(gma500, android), it seems
> that they are firstly added into drivers/staging/XXX and then moved to
> drivers/XXX after the driver becomes mature.
> So we refer to this method to upstream ACRN driver part.

Those two examples are probably the worst examples to ever look at :)

The code quality of those submissions was horrible, gma500 took a very
long time to clean up and there are parts of the android code that are
still in staging to this day.

> If the new driver can also be added by skipping the staging approach,
> we will refine it and then submit it in normal process.

That is the normal process, staging should not be needed at all for any
code.  It is a fall-back for when the company involved has no idea of
how to upstream their code, which should NOT be the case here.

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [RFC PATCH 10/15] drivers/acrn: add interrupt injection support

2019-08-18 Thread Zhao, Yakui



On 2019年08月16日 21:12, Dan Carpenter wrote:

On Fri, Aug 16, 2019 at 10:25:51AM +0800, Zhao Yakui wrote:

+   case IC_VM_INTR_MONITOR: {
+   struct page *page;
+
+   ret = get_user_pages_fast(ioctl_param, 1, 1, );
+   if (unlikely(ret != 1) || !page) {


Not required.


Do you mean that it is enough to check the condition of "ret != 1"?
OK. It will be removed.





+   pr_err("acrn-dev: failed to pin intr hdr buffer!\n");
+   return -ENOMEM;
+   }
+
+   ret = hcall_vm_intr_monitor(vm->vmid, page_to_phys(page));
+   if (ret < 0) {
+   pr_err("acrn-dev: monitor intr data err=%ld\n", ret);
+   return -EFAULT;
+   }
+   break;
+   }
+


regards,
dan carpenter


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [RFC PATCH 11/15] drivers/acrn: add the support of handling emulated ioreq

2019-08-18 Thread Zhao, Yakui



On 2019年08月16日 21:39, Dan Carpenter wrote:

On Fri, Aug 16, 2019 at 10:25:52AM +0800, Zhao Yakui wrote:

+int acrn_ioreq_create_client(unsigned short vmid,
+ioreq_handler_t handler,
+void *client_priv,
+char *name)
+{
+   struct acrn_vm *vm;
+   struct ioreq_client *client;
+   int client_id;
+
+   might_sleep();
+
+   vm = find_get_vm(vmid);
+   if (unlikely(!vm || !vm->req_buf)) {
+   pr_err("acrn-ioreq: failed to find vm from vmid %d\n", vmid);
+   put_vm(vm);
+   return -EINVAL;
+   }
+
+   client_id = alloc_client();
+   if (unlikely(client_id < 0)) {
+   pr_err("acrn-ioreq: vm[%d] failed to alloc ioreq client\n",
+  vmid);
+   put_vm(vm);
+   return -EINVAL;
+   }
+
+   client = acrn_ioreq_get_client(client_id);
+   if (unlikely(!client)) {
+   pr_err("failed to get the client.\n");
+   put_vm(vm);
+   return -EINVAL;


Do we need to clean up the alloc_client() allocation?


Thanks for the review.

The function of acrn_iocreq_get_client is used to return the client for 
the given client_id. (The ref_count of client is also added). If it is 
NULL, it indicates that it is already released in another place.


In the function of acrn_ioreq_create_client, we don't need to clean up 
the alloc_client as it always exists in course of creating_client.




regards,
dan carpenter


+   }
+
+   if (handler) {
+   client->handler = handler;
+   client->acrn_create_kthread = true;
+   }
+
+   client->ref_vm = vm;
+   client->vmid = vmid;
+   client->client_priv = client_priv;
+   if (name)
+   strncpy(client->name, name, sizeof(client->name) - 1);
+   rwlock_init(>range_lock);
+   INIT_LIST_HEAD(>range_list);
+   init_waitqueue_head(>wq);
+
+   /* When it is added to ioreq_client_list, the refcnt is increased */
+   spin_lock_bh(>ioreq_client_lock);
+   list_add(>list, >ioreq_client_list);
+   spin_unlock_bh(>ioreq_client_lock);
+
+   pr_info("acrn-ioreq: created ioreq client %d\n", client_id);
+
+   return client_id;
+}



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: media: hantro: Remove call to memset after dma_alloc_coherent

2019-08-18 Thread Tomasz Figa
Hi Hans,

On Thu, Jul 25, 2019 at 8:50 PM Boris Brezillon
 wrote:
>
> On Thu, 25 Jul 2019 08:36:02 +0530
> Hariprasad Kelam  wrote:
>
> > fix below issue reported by coccicheck
> > /drivers/staging/media/hantro/hantro_vp8.c:149:16-34: WARNING:
> > dma_alloc_coherent use in aux_buf -> cpu already zeroes out memory,  so
> > memset is not needed
> >
> > Signed-off-by: Hariprasad Kelam 
>
> Reviewed-by: Boris Brezillon 
>
> > ---
> >  drivers/staging/media/hantro/hantro_vp8.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/staging/media/hantro/hantro_vp8.c 
> > b/drivers/staging/media/hantro/hantro_vp8.c
> > index 66c4533..363ddda 100644
> > --- a/drivers/staging/media/hantro/hantro_vp8.c
> > +++ b/drivers/staging/media/hantro/hantro_vp8.c
> > @@ -151,8 +151,6 @@ int hantro_vp8_dec_init(struct hantro_ctx *ctx)
> >   if (!aux_buf->cpu)
> >   return -ENOMEM;
> >
> > - memset(aux_buf->cpu, 0, aux_buf->size);
> > -
> >   /*
> >* Allocate probability table buffer,
> >* total 1208 bytes, 4K page is far enough.
>

Is this something you will pick to your tree?

Best regards,
Tomasz
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: media: hantro: Remove call to memset after dma_alloc_coherent

2019-08-18 Thread Tomasz Figa
On Mon, Aug 19, 2019 at 1:17 PM Tomasz Figa  wrote:
>
> Hi Hans,
>
> On Thu, Jul 25, 2019 at 8:50 PM Boris Brezillon
>  wrote:
> >
> > On Thu, 25 Jul 2019 08:36:02 +0530
> > Hariprasad Kelam  wrote:
> >
> > > fix below issue reported by coccicheck
> > > /drivers/staging/media/hantro/hantro_vp8.c:149:16-34: WARNING:
> > > dma_alloc_coherent use in aux_buf -> cpu already zeroes out memory,  so
> > > memset is not needed
> > >
> > > Signed-off-by: Hariprasad Kelam 
> >
> > Reviewed-by: Boris Brezillon 
> >
> > > ---
> > >  drivers/staging/media/hantro/hantro_vp8.c | 2 --
> > >  1 file changed, 2 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/hantro/hantro_vp8.c 
> > > b/drivers/staging/media/hantro/hantro_vp8.c
> > > index 66c4533..363ddda 100644
> > > --- a/drivers/staging/media/hantro/hantro_vp8.c
> > > +++ b/drivers/staging/media/hantro/hantro_vp8.c
> > > @@ -151,8 +151,6 @@ int hantro_vp8_dec_init(struct hantro_ctx *ctx)
> > >   if (!aux_buf->cpu)
> > >   return -ENOMEM;
> > >
> > > - memset(aux_buf->cpu, 0, aux_buf->size);
> > > -
> > >   /*
> > >* Allocate probability table buffer,
> > >* total 1208 bytes, 4K page is far enough.
> >
>
> Is this something you will pick to your tree?

Ah, sorry, this is already applied. Not sure why searching for it the
first time didn't show anything. I guess I need to start repeating my
searches by default. Sorry for the noise.

Best regards,
Tomasz
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [RFC PATCH 04/15] drivers/acrn: add the basic framework of acrn char device driver

2019-08-18 Thread Zhao, Yakui



On 2019年08月16日 15:05, Greg KH wrote:

On Fri, Aug 16, 2019 at 10:25:45AM +0800, Zhao Yakui wrote:

ACRN hypervisor service module is the important middle layer that allows
the Linux kernel to communicate with the ACRN hypervisor. It includes
the management of virtualized CPU/memory/device/interrupt for other ACRN
guest. The user-space applications can use the provided ACRN ioctls to
interact with ACRN hypervisor through different hypercalls.

Add one basic framework firstly and the following patches will
add the corresponding implementations, which includes the management of
virtualized CPU/memory/interrupt and the emulation of MMIO/IO/PCI access.
The device file of /dev/acrn_hsm can be accessed in user-space to
communicate with ACRN module.

Co-developed-by: Jason Chen CJ 
Signed-off-by: Jason Chen CJ 
Co-developed-by: Jack Ren 
Signed-off-by: Jack Ren 
Co-developed-by: Mingqiang Chi 
Signed-off-by: Mingqiang Chi 
Co-developed-by: Liu Shuo 
Signed-off-by: Liu Shuo 
Signed-off-by: Zhao Yakui 
---
  drivers/staging/Kconfig |   2 +


Also, your subject line for all of these patches are wrong, it is not
drivers/acrn :(


Thanks for the pointing out it.

It will be fixed.



And you forgot to cc: the staging maintainer :(


Do you mean that the maintainer of staging subsystem is also added in 
the patch commit log?





As I have said with NUMEROUS Intel patches in the past, I now refuse to
take patches from you all WITHOUT having it signed-off-by someone from
the Intel "OTC" group (or whatever the Intel Linux group is called these
days).  They are a resource you can not ignore, and if you do, you just
end up making the rest of the kernel community grumpy by having us do
their work for them :(

Please work with them.


OK. I will work with some peoples in OTC group to prepare the better 
ACRN driver.




greg k-h


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [RFC PATCH 00/15] acrn: add the ACRN driver module

2019-08-18 Thread Zhao, Yakui



On 2019年08月16日 15:03, Greg KH wrote:

On Fri, Aug 16, 2019 at 08:39:25AM +0200, Borislav Petkov wrote:

On Fri, Aug 16, 2019 at 10:25:41AM +0800, Zhao Yakui wrote:

The first three patches are the changes under x86/acrn, which adds the
required APIs for the driver and reports the X2APIC caps.
The remaining patches add the ACRN driver module, which accepts the ioctl
from user-space and then communicate with the low-level ACRN hypervisor
by using hypercall.


I have a problem with that: you're adding interfaces to arch/x86/ and
its users go into staging. Why? Why not directly put the driver where
it belongs, clean it up properly and submit it like everything else is
submitted?

I don't want to have stuff in arch/x86/ which is used solely by code in
staging and the latter is lingering there indefinitely because no one is
cleaning it up...


I agree, stuff in drivers/staging/ must be self-contained, with no
changes outside of the code's subdirectory needed in order for it to
work.  That way it is trivial for us to delete it when it never gets
cleaned up :)


Thanks for pointing out the rule of drivers/staging.
The acrn staging driver is one self-contained driver. But it has some 
dependency on arch/x86/acrn and need to call the APIs in arch/x86/acrn.


If there is no driver,  the API without user had better not be added.
If API is not added,  the driver can't be compiled correctly.
The ACRN driver is one new driver. Maybe it will have some bugs and not 
be mature. So we want to add the driver as the staging.


What is the better approach to handle such scenario?



You never say _why_ this should go into drivers/staging/, nor do you
have a TODO file like all other staging code that explains exactly what
needs to be done to get it out of there.


Ok. The TODO file will be added in next version.




thanks,

greg k-h


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [RFC PATCH 00/15] acrn: add the ACRN driver module

2019-08-18 Thread Zhao, Yakui



On 2019年08月16日 14:39, Borislav Petkov wrote:

On Fri, Aug 16, 2019 at 10:25:41AM +0800, Zhao Yakui wrote:

The first three patches are the changes under x86/acrn, which adds the
required APIs for the driver and reports the X2APIC caps.
The remaining patches add the ACRN driver module, which accepts the ioctl
from user-space and then communicate with the low-level ACRN hypervisor
by using hypercall.


I have a problem with that: you're adding interfaces to arch/x86/ and
its users go into staging. Why? Why not directly put the driver where
it belongs, clean it up properly and submit it like everything else is
submitted?


Thanks for your reply and the concern.

After taking a look at several driver examples(gma500, android), it 
seems that they are firstly added into drivers/staging/XXX and then 
moved to drivers/XXX after the driver becomes mature.

So we refer to this method to upstream ACRN driver part.

If the new driver can also be added by skipping the staging approach,
we will refine it and then submit it in normal process.


I don't want to have stuff in arch/x86/ which is used solely by code in
staging and the latter is lingering there indefinitely because no one is
cleaning it up...



The ACRN driver will be the only user of the added APIs in x86/acrn. 
Without the APIs in x86/acrn, the driver can't add the driver-specifc 
upcall notification ISR or call the hypercall.


Not sure whether it can be sent in two patch sets?
The first is to add the required APIs for ACRN driver.
The second is to add the ACRN driver

Thanks
   Yakui
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [BUG] removing and reinserting imx-media causes kernel to explode

2019-08-18 Thread Steve Longerbeam



On 8/14/19 2:25 AM, Russell King - ARM Linux admin wrote:

I just did this:

rmmod imx-media
modprobe imx-media

and was greeted by the below kernel messages.


Yes this needs fixing, the CSI needs to check first that it is already 
registered before going through the ->registered() steps. Posting a patch.



   I don't think this has
been the first issue I found with the iMX media stuff involving a module
unload/reload cycle - may I suggest that this is added to the testing
regime for this code?  Thanks.


I do test module unload/reload cycles, but v4l2-async does not support 
re-registering subdevices unless the subdevice is basically completely 
removed and re-probed, so there won't be a working media device if only 
imx-media is reloaded. So I have always unloaded/reloaded all modules 
for every entity in the graph, i.e.:


rmmod imx6_media imx6_media_csi imx6_mipi_csi2 ov5640 video_mux 
imx_media_common


(replace ov5640 with your imx219 sensor).

But I'll make sure to test single module unload/reload cycles in the future.

But note after applying the patch mentioned above to CSI ->registered() 
callback, there are list corruption backtraces, see [1]. The root cause 
is that both media_device_register_entity() and media_entity_pads_init() 
add the same graph objects for the entity's pads, so duplicate pad 
objects are added to the media device pads list. Removing the pad object 
creation in media_device_register_entity() fixes the list corruption. 
Sending a patch for that also.


This is a problem for any entity that sets its ->num_pads to a non-zero 
value before media_device_register_entity() is called. For example, the 
following will produce the same list corruption backtrace:


rmmod video-mux
modprobe video-mux
rmmod video-mux

Steve

[1]
rmmod imx6-media
modprobe imx6-media
rmmod imx6-media

[  249.387953] WARNING: CPU: 2 PID: 843 at lib/list_debug.c:53 
__list_del_entry_valid+0xa0/0xdc
[  249.396442] list_del corruption. prev->next should be e8fb0510, but 
was e93b5914
[  249.404076] Modules linked in: imx6_media_csi(C) imx6_media(C-) 
imx6_mipi_csi2(C) bnep dw_hdmi_ahb_audio dw_hdmi_cec ov5640 mux_mmio 
video_mux mux_core dw_hdmi_imx dw_hdmi coda_vpu cec imx_vdoa 
videobuf2_vmalloc imx_media_common(C) v4l2_fwnode imx_ldb imxdrm 
imx_ipu_v3 [last unloaded: imx6_media_csi]
[  249.430956] CPU: 2 PID: 843 Comm: rmmod Tainted: G C    
5.3.0-rc4-01115-g62119fd20fda #5

[  249.440115] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[  249.446689] [] (unwind_backtrace) from [] 
(show_stack+0x10/0x14)
[  249.454462] [] (show_stack) from [] 
(dump_stack+0xd8/0x110)
[  249.461804] [] (dump_stack) from [] 
(__warn+0xe0/0x10c)
[  249.468789] [] (__warn) from [] 
(warn_slowpath_fmt+0x44/0x6c)
[  249.476301] [] (warn_slowpath_fmt) from [] 
(__list_del_entry_valid+0xa0/0xdc)
[  249.485207] [] (__list_del_entry_valid) from [] 
(media_gobj_destroy.part.4+0x30/0x64)
[  249.494802] [] (media_gobj_destroy.part.4) from 
[] (__media_device_unregister_entity+0xa8/0xcc)
[  249.505259] [] (__media_device_unregister_entity) from 
[] (media_device_unregister_entity+0x2c/0x38)
[  249.516157] [] (media_device_unregister_entity) from 
[] (v4l2_device_unregister_subdev+0x90/0xb4)
[  249.526793] [] (v4l2_device_unregister_subdev) from 
[] (v4l2_async_cleanup+0x10/0x3c)
[  249.536382] [] (v4l2_async_cleanup) from [] 
(v4l2_async_notifier_unbind_all_subdevs+0x9c/0x10c)
[  249.546840] [] (v4l2_async_notifier_unbind_all_subdevs) 
from [] (v4l2_async_notifier_unbind_all_subdevs+0x6c/0x10c)
[  249.559035] [] (v4l2_async_notifier_unbind_all_subdevs) 
from [] (__v4l2_async_notifier_unregister.part.4+0xc/0x44)
[  249.571140] [] (__v4l2_async_notifier_unregister.part.4) 
from [] (v4l2_async_notifier_unregister+0x30/0x50)
[  249.582665] [] (v4l2_async_notifier_unregister) from 
[] (imx_media_remove+0x20/0x54 [imx6_media])
[  249.593389] [] (imx_media_remove [imx6_media]) from 
[] (platform_drv_remove+0x20/0x40)
[  249.603068] [] (platform_drv_remove) from [] 
(device_release_driver_internal+0xdc/0x1ac)
[  249.612917] [] (device_release_driver_internal) from 
[] (driver_detach+0x44/0x80)
[  249.622164] [] (driver_detach) from [] 
(bus_remove_driver+0x5c/0xd8)
[  249.630287] [] (bus_remove_driver) from [] 
(sys_delete_module+0x17c/0x20c)
[  249.638926] [] (sys_delete_module) from [] 
(ret_fast_syscall+0x0/0x28)

[  249.647205] Exception stack(0xe90d5fa8 to 0xe90d5ff0)
[  249.652278] 5fa0:   beed5d6c 0003 01401134 
0800 4f13b6f4 2002
[  249.660475] 5fc0: beed5d6c 0003 beed5b80 0081 beed5e78 
0001  014010f8

[  249.668669] 5fe0: 0003b2c4 beed5b4c 0001f248 4f1012dc
[  249.673859] irq event stamp: 4113
[  249.677267] hardirqs last  enabled at (4131): [] 
console_unlock+0x408/0x5f8
[  249.685125] hardirqs last disabled at (4138): [] 
console_unlock+0x88/0x5f8
[  249.692970] softirqs last  enabled at (4154): [] 
__do_softirq+0x360/0x524
[  249.700735] softirqs last disabled at 

Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Gao Xiang
Hi all,

On Mon, Aug 19, 2019 at 02:16:55AM +0800, Gao Xiang wrote:
> Hi Hch,
> 
> On Sun, Aug 18, 2019 at 10:47:02AM -0700, Christoph Hellwig wrote:
> > On Sun, Aug 18, 2019 at 10:29:38AM -0700, Eric Biggers wrote:
> > > Not sure what you're even disagreeing with, as I *do* expect new 
> > > filesystems to
> > > be held to a high standard, and to be written with the assumption that the
> > > on-disk data may be corrupted or malicious.  We just can't expect the bar 
> > > to be
> > > so high (e.g. no bugs) that it's never been attained by *any* filesystem 
> > > even
> > > after years/decades of active development.  If the developers were 
> > > careful, the
> > > code generally looks robust, and they are willing to address such bugs as 
> > > they
> > > are found, realistically that's as good as we can expect to get...
> >
> > Well, the impression I got from Richards quick look and the reply to it is
> > that there is very little attempt to validate the ondisk data structure
> > and there is absolutely no priority to do so.  Which is very different
> > from there is a bug or two here and there.
> 
> As my second reply to Richard, I didn't fuzz all the on-disk fields for EROFS.
> and as my reply to Richard / Greg, current EROFS is used on the top of 
> dm-verity.
> 
> I cannot say how well EROFS will be performed on malformed images (and you can
> also find the bug richard pointed out is a miswritten break->continue by 
> myself).
> 
> I posted the upstream EROFS post on July 4, 2019 and a month and a half later,
> no one can tell me (yes, thanks for kind people reply me about their 
> suggestion)
> what we should do next (you can see these emails, I sent many times) to meet
> the minimal upstream requirements and rare people can even dip into my code.
> 
> That is all I want to say. I will work on autofuzz these days, and I want to
> know how to meet your requirements on this (you can tell us your standard,
> how well should we do).
> 
> OK, you don't reply to my post once, I have no idea how to get your first 
> reply.

I have made a simple fuzzer to inject messy in inode metadata,
dir data, compressed indexes and super block,
https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git/commit/?h=experimental-fuzzer

I am testing with some given dirs and the following script.
Does it look reasonable?

# !/bin/bash

mkdir -p mntdir

for ((i=0; i<1000; ++i)); do
mkfs/mkfs.erofs -F$i testdir_fsl.fuzz.img testdir_fsl > /dev/null 2>&1
umount mntdir
mount -t erofs -o loop testdir_fsl.fuzz.img mntdir
for j in `find mntdir -type f`; do
md5sum $j > /dev/null
done
done

Thanks,
Gao Xiang

> 
> Thanks,
> Gao Xiang
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2] staging: iio: accel: adis16240: Improve readability on write_raw function

2019-08-18 Thread Jonathan Cameron
On Wed, 14 Aug 2019 06:56:18 +
"Ardelean, Alexandru"  wrote:

> On Tue, 2019-08-13 at 16:31 -0300, Rodrigo Ribeiro wrote:
> > [External]
> > 
> > Replace shift and minus operation by GENMASK macro and remove the local
> > variables used to store intermediate data.
> >   
> 
> Reviewed-by: Alexandru Ardelean 
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan

> 
> > Signed-off-by: Rodrigo Ribeiro Carvalho 
> > ---
> > v2:
> >- Leave switch statement instead of replace by if statement
> >  drivers/staging/iio/accel/adis16240.c | 5 +
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/accel/adis16240.c 
> > b/drivers/staging/iio/accel/adis16240.c
> > index 62f4b3b1b457..82099db4bf0c 100644
> > --- a/drivers/staging/iio/accel/adis16240.c
> > +++ b/drivers/staging/iio/accel/adis16240.c
> > @@ -309,15 +309,12 @@ static int adis16240_write_raw(struct iio_dev 
> > *indio_dev,
> >long mask)
> >  {
> > struct adis *st = iio_priv(indio_dev);
> > -   int bits = 10;
> > -   s16 val16;
> > u8 addr;
> >  
> > switch (mask) {
> > case IIO_CHAN_INFO_CALIBBIAS:
> > -   val16 = val & ((1 << bits) - 1);
> > addr = adis16240_addresses[chan->scan_index][0];
> > -   return adis_write_reg_16(st, addr, val16);
> > +   return adis_write_reg_16(st, addr, val & GENMASK(9, 0));
> > }
> > return -EINVAL;
> >  }  

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


[PATCH] staging: rts5208: remove redundant assignment to retval

2019-08-18 Thread Colin King
From: Colin Ian King 

Variable retval is initialized to a value that is never read and it
is re-assigned later. The initialization is redundant and can be
removed.

Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King 
---
 drivers/staging/rts5208/ms.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rts5208/ms.c b/drivers/staging/rts5208/ms.c
index 1128eec3bd08..e853fa9cc950 100644
--- a/drivers/staging/rts5208/ms.c
+++ b/drivers/staging/rts5208/ms.c
@@ -3842,7 +3842,7 @@ int mg_set_leaf_id(struct scsi_cmnd *srb, struct 
rtsx_chip *chip)
 
 int mg_get_local_EKB(struct scsi_cmnd *srb, struct rtsx_chip *chip)
 {
-   int retval = STATUS_FAIL;
+   int retval;
int bufflen;
unsigned int lun = SCSI_LUN(srb);
u8 *buf = NULL;
-- 
2.20.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Gao Xiang
Hi Richard,

On Sun, Aug 18, 2019 at 08:00:28PM +0200, Richard Weinberger wrote:
> - Ursprüngliche Mail -
> > Von: "tytso" 
> > An: "richard" 
> > CC: "Greg Kroah-Hartman" , "Gao Xiang" 
> > , "Jan Kara" , "Chao
> > Yu" , "Dave Chinner" , "David 
> > Sterba" , "Miao Xie"
> > , "devel" , "Stephen 
> > Rothwell" , "Darrick"
> > , "Christoph Hellwig" , "Amir 
> > Goldstein" ,
> > "linux-erofs" , "Al Viro" 
> > , "Jaegeuk Kim" ,
> > "linux-kernel" , "Li Guifu" 
> > , "Fang Wei" ,
> > "Pavel Machek" , "linux-fsdevel" 
> > , "Andrew Morton"
> > , "torvalds" 
> > Gesendet: Sonntag, 18. August 2019 19:46:21
> > Betreff: Re: [PATCH] erofs: move erofs out of staging
>
> > On Sun, Aug 18, 2019 at 07:06:40PM +0200, Richard Weinberger wrote:
> >> > So holding a file system like EROFS to a higher standard than say,
> >> > ext4, xfs, or btrfs hardly seems fair.
> >>
> >> Nobody claimed that.
> >
> > Pointing out that erofs has issues in this area when Gao Xiang is
> > asking if erofs can be moved out of staging and join the "official
> > clubhouse" of file systems could certainly be reasonable interpreted
> > as such.  Reporting such vulnerablities are a good thing, and
> > hopefully all file system maintainers will welcome them.  Doing them
> > on a e-mail thread about promoting out of erofs is certainly going to
> > lead to inferences of a double standard.
>
> Well, this was not at all my intention.
> erofs raised my attention and instead of wasting a new thread
> I answered here and reported what I found while looking at it.
> That's all.

Thank you very much, EROFS finally has some real concern
after a quite long time. I will do that but I really want
to upstream for 5.4LTS and hope to get your further report.

Thanks,
Gao Xiang

>
> Thanks,
> //richard
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


[PATCH] staging: kpc2000: kpc2000_i2c: Fix different address spaces warnings

2019-08-18 Thread Eduardo Barretto
This patch fixes the following sparse warnings:

kpc2000_i2c.c:137: warning: incorrect type in argument 1
   (different address spaces)
kpc2000_i2c.c:137:expected void const volatile [noderef]  *addr
kpc2000_i2c.c:137:got void *
kpc2000_i2c.c:146: warning: incorrect type in argument 2
   (different address spaces)
kpc2000_i2c.c:146:expected void volatile [noderef]  *addr
kpc2000_i2c.c:146:got void *
kpc2000_i2c.c:147: warning: incorrect type in argument 1
   (different address spaces)
kpc2000_i2c.c:147:expected void const volatile [noderef]  *addr
kpc2000_i2c.c:147:got void *
kpc2000_i2c.c:166: warning: incorrect type in argument 1
   (different address spaces)
kpc2000_i2c.c:166:expected void const volatile [noderef]  *addr
kpc2000_i2c.c:166:got void *
kpc2000_i2c.c:166: warning: incorrect type in argument 2
  (different address spaces)
kpc2000_i2c.c:166:expected void volatile [noderef]  *addr
kpc2000_i2c.c:166:got void *
kpc2000_i2c.c:168: warning: incorrect type in argument 1
   (different address spaces)
kpc2000_i2c.c:168:expected void const volatile [noderef]  *addr
kpc2000_i2c.c:168:got void *
kpc2000_i2c.c:168: warning: incorrect type in argument 2
   (different address spaces)
kpc2000_i2c.c:168:expected void volatile [noderef]  *addr
kpc2000_i2c.c:168:got void *
kpc2000_i2c.c:171: warning: incorrect type in argument 1
   (different address spaces)
kpc2000_i2c.c:171:expected void const volatile [noderef]  *addr
kpc2000_i2c.c:171:got void *
kpc2000_i2c.c:174: warning: incorrect type in argument 2
   (different address spaces)
kpc2000_i2c.c:174:expected void volatile [noderef]  *addr
kpc2000_i2c.c:174:got void *
kpc2000_i2c.c:193: warning: incorrect type in argument 2
   (different address spaces)
kpc2000_i2c.c:193:expected void volatile [noderef]  *addr
kpc2000_i2c.c:193:got void *
kpc2000_i2c.c:194: warning: incorrect type in argument 1
   (different address spaces)
kpc2000_i2c.c:194:expected void const volatile [noderef]  *addr
kpc2000_i2c.c:194:got void *
kpc2000_i2c.c:214: warning: incorrect type in argument 2
  (different address spaces)
kpc2000_i2c.c:214:expected void volatile [noderef]  *addr
kpc2000_i2c.c:214:got void *
kpc2000_i2c.c:219: warning: incorrect type in argument 1
  (different address spaces)
kpc2000_i2c.c:219:expected void const volatile [noderef]  *addr
kpc2000_i2c.c:219:got void *
kpc2000_i2c.c:226: warning: incorrect type in argument 2
  (different address spaces)
kpc2000_i2c.c:226:expected void volatile [noderef]  *addr
kpc2000_i2c.c:226:got void *
kpc2000_i2c.c:238: warning: incorrect type in argument 1
   (different address spaces)
kpc2000_i2c.c:238:expected void const volatile [noderef]  *addr
kpc2000_i2c.c:238:got void *
kpc2000_i2c.c:244: warning: incorrect type in argument 2
  (different address spaces)
kpc2000_i2c.c:244:expected void volatile [noderef]  *addr
kpc2000_i2c.c:244:got void *
kpc2000_i2c.c:252: warning: incorrect type in argument 1
  (different address spaces)
kpc2000_i2c.c:252:expected void const volatile [noderef]  *addr
kpc2000_i2c.c:252:got void *
kpc2000_i2c.c:257: warning: incorrect type in argument 2
  (different address spaces)
kpc2000_i2c.c:257:expected void volatile [noderef]  *addr
kpc2000_i2c.c:257:got void *
kpc2000_i2c.c:259: warning: incorrect type in argument 2
   (different address spaces)
kpc2000_i2c.c:259:expected void volatile [noderef]  *addr
kpc2000_i2c.c:259:got void *
kpc2000_i2c.c:267: warning: incorrect type in argument 1
   (different address spaces)
kpc2000_i2c.c:267:expected void const volatile [noderef]  *addr
kpc2000_i2c.c:267:got void *
kpc2000_i2c.c:273: warning: incorrect type in argument 1
   (different address spaces)
kpc2000_i2c.c:273:expected void const volatile [noderef]  *addr
kpc2000_i2c.c:273:got void *
kpc2000_i2c.c:293: warning: incorrect type in argument 2
   (different address spaces)
kpc2000_i2c.c:293:expected void volatile [noderef]  *addr
kpc2000_i2c.c:293:got void *
kpc2000_i2c.c:294: warning: incorrect type in argument 2
   (different address spaces)
kpc2000_i2c.c:294:expected void volatile [noderef]  *addr

Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Gao Xiang
Hi Hch,

On Sun, Aug 18, 2019 at 10:47:02AM -0700, Christoph Hellwig wrote:
> On Sun, Aug 18, 2019 at 10:29:38AM -0700, Eric Biggers wrote:
> > Not sure what you're even disagreeing with, as I *do* expect new 
> > filesystems to
> > be held to a high standard, and to be written with the assumption that the
> > on-disk data may be corrupted or malicious.  We just can't expect the bar 
> > to be
> > so high (e.g. no bugs) that it's never been attained by *any* filesystem 
> > even
> > after years/decades of active development.  If the developers were careful, 
> > the
> > code generally looks robust, and they are willing to address such bugs as 
> > they
> > are found, realistically that's as good as we can expect to get...
>
> Well, the impression I got from Richards quick look and the reply to it is
> that there is very little attempt to validate the ondisk data structure
> and there is absolutely no priority to do so.  Which is very different
> from there is a bug or two here and there.

As my second reply to Richard, I didn't fuzz all the on-disk fields for EROFS.
and as my reply to Richard / Greg, current EROFS is used on the top of 
dm-verity.

I cannot say how well EROFS will be performed on malformed images (and you can
also find the bug richard pointed out is a miswritten break->continue by 
myself).

I posted the upstream EROFS post on July 4, 2019 and a month and a half later,
no one can tell me (yes, thanks for kind people reply me about their suggestion)
what we should do next (you can see these emails, I sent many times) to meet
the minimal upstream requirements and rare people can even dip into my code.

That is all I want to say. I will work on autofuzz these days, and I want to
know how to meet your requirements on this (you can tell us your standard,
how well should we do).

OK, you don't reply to my post once, I have no idea how to get your first reply.

Thanks,
Gao Xiang

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Richard Weinberger
- Ursprüngliche Mail -
> Von: "tytso" 
> An: "richard" 
> CC: "Greg Kroah-Hartman" , "Gao Xiang" 
> , "Jan Kara" , "Chao
> Yu" , "Dave Chinner" , "David 
> Sterba" , "Miao Xie"
> , "devel" , "Stephen 
> Rothwell" , "Darrick"
> , "Christoph Hellwig" , "Amir 
> Goldstein" ,
> "linux-erofs" , "Al Viro" 
> , "Jaegeuk Kim" ,
> "linux-kernel" , "Li Guifu" 
> , "Fang Wei" ,
> "Pavel Machek" , "linux-fsdevel" 
> , "Andrew Morton"
> , "torvalds" 
> Gesendet: Sonntag, 18. August 2019 19:46:21
> Betreff: Re: [PATCH] erofs: move erofs out of staging

> On Sun, Aug 18, 2019 at 07:06:40PM +0200, Richard Weinberger wrote:
>> > So holding a file system like EROFS to a higher standard than say,
>> > ext4, xfs, or btrfs hardly seems fair.
>> 
>> Nobody claimed that.
> 
> Pointing out that erofs has issues in this area when Gao Xiang is
> asking if erofs can be moved out of staging and join the "official
> clubhouse" of file systems could certainly be reasonable interpreted
> as such.  Reporting such vulnerablities are a good thing, and
> hopefully all file system maintainers will welcome them.  Doing them
> on a e-mail thread about promoting out of erofs is certainly going to
> lead to inferences of a double standard.

Well, this was not at all my intention.
erofs raised my attention and instead of wasting a new thread
I answered here and reported what I found while looking at it.
That's all.

Thanks,
//richard
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Christoph Hellwig
On Sun, Aug 18, 2019 at 10:29:38AM -0700, Eric Biggers wrote:
> Not sure what you're even disagreeing with, as I *do* expect new filesystems 
> to
> be held to a high standard, and to be written with the assumption that the
> on-disk data may be corrupted or malicious.  We just can't expect the bar to 
> be
> so high (e.g. no bugs) that it's never been attained by *any* filesystem even
> after years/decades of active development.  If the developers were careful, 
> the
> code generally looks robust, and they are willing to address such bugs as they
> are found, realistically that's as good as we can expect to get...

Well, the impression I got from Richards quick look and the reply to it is
that there is very little attempt to validate the ondisk data structure
and there is absolutely no priority to do so.  Which is very different
from there is a bug or two here and there.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Theodore Y. Ts'o
On Sun, Aug 18, 2019 at 07:06:40PM +0200, Richard Weinberger wrote:
> > So holding a file system like EROFS to a higher standard than say,
> > ext4, xfs, or btrfs hardly seems fair.
> 
> Nobody claimed that.

Pointing out that erofs has issues in this area when Gao Xiang is
asking if erofs can be moved out of staging and join the "official
clubhouse" of file systems could certainly be reasonable interpreted
as such.  Reporting such vulnerablities are a good thing, and
hopefully all file system maintainers will welcome them.  Doing them
on a e-mail thread about promoting out of erofs is certainly going to
lead to inferences of a double standard.

Cheers,

- Ted
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Theodore Y. Ts'o
On Sun, Aug 18, 2019 at 08:58:12AM -0700, Christoph Hellwig wrote:
> On Sun, Aug 18, 2019 at 11:11:54AM -0400, Theodore Y. Ts'o wrote:
> > Note that of the mainstream file systems, ext4 and xfs don't guarantee
> > that it's safe to blindly take maliciously provided file systems, such
> > as those provided by a untrusted container, and mount it on a file
> > system without problems.  As I recall, one of the XFS developers
> > described file system fuzzing reports as a denial of service attack on
> > the developers.
> 
> I think this greatly misrepresents the general attitute of the XFS
> developers.  We take sanity checks for the modern v5 on disk format
> very series, and put a lot of effort into handling corrupted file
> systems as good as possible, although there are of course no guaranteeѕ.
> 
> The quote that you've taken out of context is for the legacy v4 format
> that has no checksums and other integrity features.

Actually, what Prof. Kim's research group was doing was taking the
latest file system formats (for ext4 and xfs) and fixing up the
checksum after fuzzing the metadata blocks.  The goal was to find
potential security vulnerabilities, not to see if file systems would
crash if fed invalid input.  At least for ext4, at least one of
Prof. Kim's fuzzing results was one that that I believe could have
been leveraged into a stack overflow attack.  I can't speak to his
results with respect to XFS, since I didn't look at them.

Cheers,

- Ted
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Eric Biggers
On Sun, Aug 18, 2019 at 09:22:01AM -0700, Christoph Hellwig wrote:
> On Sun, Aug 18, 2019 at 09:16:38AM -0700, Eric Biggers wrote:
> > Ted's observation was about maliciously-crafted filesystems, though, so
> > integrity-only features such as metadata checksums are irrelevant.  Also the
> > filesystem version is irrelevant; anything accepted by the kernel code 
> > (even if
> 
> I think allowing users to mount file systems (any of ours) without
> privilege is a rather bad idea.  But that doesn't mean we should not be
> as robust as we can.  Optionally disabling support for legacy formats
> at compile and/or runtime is something we should actively look into as
> well.
> 
> > it's legacy/deprecated) is open attack surface.
> > 
> > I personally consider it *mandatory* that we deal with this stuff.  But I 
> > can
> > understand that we don't do a good job at it, so we shouldn't hold a new
> > filesystem to an unfairly high standard relative to other filesystems...
> 
> I very much disagree.  We can't really force anyone to fix up old file
> systems.  But we can very much hold new ones to (slightly) higher
> standards.  Thats the only way to get the average quality up.  Some as
> for things like code style - we can't magically fix up all old stuff,
> but we can and usually do hold new code to higher standards.  (Often not
> to standards as high as I'd personally prefer, btw).

Not sure what you're even disagreeing with, as I *do* expect new filesystems to
be held to a high standard, and to be written with the assumption that the
on-disk data may be corrupted or malicious.  We just can't expect the bar to be
so high (e.g. no bugs) that it's never been attained by *any* filesystem even
after years/decades of active development.  If the developers were careful, the
code generally looks robust, and they are willing to address such bugs as they
are found, realistically that's as good as we can expect to get...

- Eric
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Richard Weinberger
- Ursprüngliche Mail -
> So holding a file system like EROFS to a higher standard than say,
> ext4, xfs, or btrfs hardly seems fair.

Nobody claimed that.

Thanks,
//richard
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [GIT PULL] Staging/IIO driver fixes for 5.3-rc5

2019-08-18 Thread pr-tracker-bot
The pull request you sent on Sun, 18 Aug 2019 10:57:12 +0200:

> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 
> tags/staging-5.3-rc5

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/ae1a616af36e5ad0726407b76feed5060a424744

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Gao Xiang
Hi Hch,

On Sun, Aug 18, 2019 at 09:22:01AM -0700, Christoph Hellwig wrote:
> On Sun, Aug 18, 2019 at 09:16:38AM -0700, Eric Biggers wrote:
> > Ted's observation was about maliciously-crafted filesystems, though, so
> > integrity-only features such as metadata checksums are irrelevant.  Also the
> > filesystem version is irrelevant; anything accepted by the kernel code 
> > (even if
> 
> I think allowing users to mount file systems (any of ours) without
> privilege is a rather bad idea.  But that doesn't mean we should not be
> as robust as we can.  Optionally disabling support for legacy formats
> at compile and/or runtime is something we should actively look into as
> well.
> 
> > it's legacy/deprecated) is open attack surface.
> > 
> > I personally consider it *mandatory* that we deal with this stuff.  But I 
> > can
> > understand that we don't do a good job at it, so we shouldn't hold a new
> > filesystem to an unfairly high standard relative to other filesystems...
> 
> I very much disagree.  We can't really force anyone to fix up old file
> systems.  But we can very much hold new ones to (slightly) higher
> standards.  Thats the only way to get the average quality up.  Some as
> for things like code style - we can't magically fix up all old stuff,
> but we can and usually do hold new code to higher standards.  (Often not
> to standards as high as I'd personally prefer, btw).

I personally don't want to discuss about other fses here...

I think XFS developers do great jobs all the time and
EROFS is a simple file system compared with these
generic file systems.

I can promise you that our team will fix bug reports in time, and
I personally think the current EROFS code is not as bad as a bullsh**t...

If you have some time, I'm very happy if you can take some of
your precious time on our work...

Thanks,
Gao Xiang


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Christoph Hellwig
On Sun, Aug 18, 2019 at 09:16:38AM -0700, Eric Biggers wrote:
> Ted's observation was about maliciously-crafted filesystems, though, so
> integrity-only features such as metadata checksums are irrelevant.  Also the
> filesystem version is irrelevant; anything accepted by the kernel code (even 
> if

I think allowing users to mount file systems (any of ours) without
privilege is a rather bad idea.  But that doesn't mean we should not be
as robust as we can.  Optionally disabling support for legacy formats
at compile and/or runtime is something we should actively look into as
well.

> it's legacy/deprecated) is open attack surface.
> 
> I personally consider it *mandatory* that we deal with this stuff.  But I can
> understand that we don't do a good job at it, so we shouldn't hold a new
> filesystem to an unfairly high standard relative to other filesystems...

I very much disagree.  We can't really force anyone to fix up old file
systems.  But we can very much hold new ones to (slightly) higher
standards.  Thats the only way to get the average quality up.  Some as
for things like code style - we can't magically fix up all old stuff,
but we can and usually do hold new code to higher standards.  (Often not
to standards as high as I'd personally prefer, btw).
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Eric Biggers
On Sun, Aug 18, 2019 at 08:58:12AM -0700, Christoph Hellwig wrote:
> On Sun, Aug 18, 2019 at 11:11:54AM -0400, Theodore Y. Ts'o wrote:
> > Note that of the mainstream file systems, ext4 and xfs don't guarantee
> > that it's safe to blindly take maliciously provided file systems, such
> > as those provided by a untrusted container, and mount it on a file
> > system without problems.  As I recall, one of the XFS developers
> > described file system fuzzing reports as a denial of service attack on
> > the developers.
> 
> I think this greatly misrepresents the general attitute of the XFS
> developers.  We take sanity checks for the modern v5 on disk format
> very series, and put a lot of effort into handling corrupted file
> systems as good as possible, although there are of course no guaranteeѕ.
> 
> The quote that you've taken out of context is for the legacy v4 format
> that has no checksums and other integrity features.

Ted's observation was about maliciously-crafted filesystems, though, so
integrity-only features such as metadata checksums are irrelevant.  Also the
filesystem version is irrelevant; anything accepted by the kernel code (even if
it's legacy/deprecated) is open attack surface.

I personally consider it *mandatory* that we deal with this stuff.  But I can
understand that we don't do a good job at it, so we shouldn't hold a new
filesystem to an unfairly high standard relative to other filesystems...

- Eric
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Gao Xiang
Hi Ted,

On Sun, Aug 18, 2019 at 11:11:54AM -0400, Theodore Y. Ts'o wrote:
> On Sun, Aug 18, 2019 at 11:21:13AM +0200, Richard Weinberger wrote:
> > > Not to say that erofs shouldn't be worked on to fix these kinds of
> > > issues, just that it's not an unheard of thing to trust the disk image.
> > > Especially for the normal usage model of erofs, where the whole disk
> > > image is verfied before it is allowed to be mounted as part of the boot
> > > process.
> > 
> > For normal use I see no problem at all.
> > I fear distros that try to mount anything you plug into your USB.
> > 
> > At least SUSE already blacklists erofs:
> > https://github.com/openSUSE/suse-module-tools/blob/master/suse-module-tools.spec#L24
> 
> Note that of the mainstream file systems, ext4 and xfs don't guarantee
> that it's safe to blindly take maliciously provided file systems, such
> as those provided by a untrusted container, and mount it on a file
> system without problems.  As I recall, one of the XFS developers
> described file system fuzzing reports as a denial of service attack on
> the developers.  And on the ext4 side, while I try to address them, it
> is by no means considered a high priority work item, and I don't
> consider fixes of fuzzing reports to be worthy of coordinated
> disclosure or a high priority bug to fix.  (I have closed more bugs in
> this area than XFS has, although that may be that ext4 started with
> more file system format bugs than XFS; however given the time to first
> bug in 2017 using American Fuzzy Lop[1] being 5 seconds for btrfs, 10
> seconds for f2fs, 25 seconds for reiserfs, 4 minutes for ntfs, 1h45m
> for xfs, and 2h for ext4, that seems unlikely.)
> 
> [1] 
> https://events.static.linuxfound.org/sites/events/files/slides/AFL%20filesystem%20fuzzing%2C%20Vault%202016_0.pdf
> 
> So holding a file system like EROFS to a higher standard than say,
> ext4, xfs, or btrfs hardly seems fair.  There seems to be a very
> unfortunate tendancy for us to hold new file systems to impossibly
> high standards, when in fact, adding a file system to Linux should
> not, in my opinion, be a remarkable event.  We have a huge number of
> them already, many of which are barely maintained and probably have
> far worse issues than file systems trying to get into the clubhouse.
> 
> If a file system is requesting core changes to the VM or block layers,
> sure, we should care about the interfaces.  But this nitpicking about
> whether or not a file system can be trusted in what I consider to be
> COMPLETELY INSANE CONTAINER USE CASES is really not fair.

Thanks for your kind reply and understanding...

Yes, EROFS is now still like a little baby, and what I can do is to write
more code to make it grown up... but I personally cannot write bug-free
code all the time (because sometimes I could be in a low mood...)

In the past year, we already adds many error handling path for corrupted
images and handles all BUG_ONs in a more proper way... we are doing our best...

Our team will continue focusing on all bug reports from external /
internal sources and fix them all in time. and for more wider scenerios,
I'd like to build an autofuzzer tools based on erofs-utils to make EROFS
more strong as one of the next step.

Thanks,
Gao Xiang


> 
> Cheers,
> 
>   - Ted
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Christoph Hellwig
On Sun, Aug 18, 2019 at 11:11:54AM -0400, Theodore Y. Ts'o wrote:
> Note that of the mainstream file systems, ext4 and xfs don't guarantee
> that it's safe to blindly take maliciously provided file systems, such
> as those provided by a untrusted container, and mount it on a file
> system without problems.  As I recall, one of the XFS developers
> described file system fuzzing reports as a denial of service attack on
> the developers.

I think this greatly misrepresents the general attitute of the XFS
developers.  We take sanity checks for the modern v5 on disk format
very series, and put a lot of effort into handling corrupted file
systems as good as possible, although there are of course no guaranteeѕ.

The quote that you've taken out of context is for the legacy v4 format
that has no checksums and other integrity features.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


[PATCH v2] staging: android: Remove ion device tree bindings from the TODO

2019-08-18 Thread Donald Yandt
Commit 23a4388f24f5 ("staging: android: ion: Remove file ion_chunk_heap.c")
and eadbf7a34e44 ("staging: android: ion: Remove file ion_carveout_heap.c")
removed the chunk and carveout heaps from ion but left behind the device
tree bindings for them in the TODO, this patch removes it.

Signed-off-by: Donald Yandt 
---
Changes in v2:
- Referenced previous commits and described the commit in greater detail.
---
 drivers/staging/android/TODO | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
index fbf015cc6d62..767dd98fd92d 100644
--- a/drivers/staging/android/TODO
+++ b/drivers/staging/android/TODO
@@ -6,8 +6,6 @@ TODO:
 
 
 ion/
- - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
-   involve putting appropriate bindings in a memory node for Ion to find.
  - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
  - Better test framework (integration with VGEM was suggested)
 
-- 
2.21.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Theodore Y. Ts'o
On Sun, Aug 18, 2019 at 11:21:13AM +0200, Richard Weinberger wrote:
> > Not to say that erofs shouldn't be worked on to fix these kinds of
> > issues, just that it's not an unheard of thing to trust the disk image.
> > Especially for the normal usage model of erofs, where the whole disk
> > image is verfied before it is allowed to be mounted as part of the boot
> > process.
> 
> For normal use I see no problem at all.
> I fear distros that try to mount anything you plug into your USB.
> 
> At least SUSE already blacklists erofs:
> https://github.com/openSUSE/suse-module-tools/blob/master/suse-module-tools.spec#L24

Note that of the mainstream file systems, ext4 and xfs don't guarantee
that it's safe to blindly take maliciously provided file systems, such
as those provided by a untrusted container, and mount it on a file
system without problems.  As I recall, one of the XFS developers
described file system fuzzing reports as a denial of service attack on
the developers.  And on the ext4 side, while I try to address them, it
is by no means considered a high priority work item, and I don't
consider fixes of fuzzing reports to be worthy of coordinated
disclosure or a high priority bug to fix.  (I have closed more bugs in
this area than XFS has, although that may be that ext4 started with
more file system format bugs than XFS; however given the time to first
bug in 2017 using American Fuzzy Lop[1] being 5 seconds for btrfs, 10
seconds for f2fs, 25 seconds for reiserfs, 4 minutes for ntfs, 1h45m
for xfs, and 2h for ext4, that seems unlikely.)

[1] 
https://events.static.linuxfound.org/sites/events/files/slides/AFL%20filesystem%20fuzzing%2C%20Vault%202016_0.pdf

So holding a file system like EROFS to a higher standard than say,
ext4, xfs, or btrfs hardly seems fair.  There seems to be a very
unfortunate tendancy for us to hold new file systems to impossibly
high standards, when in fact, adding a file system to Linux should
not, in my opinion, be a remarkable event.  We have a huge number of
them already, many of which are barely maintained and probably have
far worse issues than file systems trying to get into the clubhouse.

If a file system is requesting core changes to the VM or block layers,
sure, we should care about the interfaces.  But this nitpicking about
whether or not a file system can be trusted in what I consider to be
COMPLETELY INSANE CONTAINER USE CASES is really not fair.

Cheers,

- Ted
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


[PATCH] staging: rtl8712: Improve naming of include hearder guards

2019-08-18 Thread Christophe JAILLET
Choose a better name for the include hearder guard used in rtl871x_io.h.
'_IO_H_' is to generic and does not match the comment after the #endif.
Use '_RTL871X_IO_H_' instead.

Also make the comments in the #endif /* XXX */ match the name used in
#ifndef.

Signed-off-by: Christophe JAILLET 
---
__RTL871X_RF_H_ could have been turned into _RTL871X_RF_H_ (one leading
'_' removed) but there does not seem to be a clear logic in the way the
header guards are named in this directory. So leave it as-is in order to
limit the diff.
---
 drivers/staging/rtl8712/rtl871x_io.h | 7 +++
 drivers/staging/rtl8712/rtl871x_rf.h | 3 +--
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl871x_io.h 
b/drivers/staging/rtl8712/rtl871x_io.h
index 28941423b7ed..c20dd5a6bbd1 100644
--- a/drivers/staging/rtl8712/rtl871x_io.h
+++ b/drivers/staging/rtl8712/rtl871x_io.h
@@ -11,8 +11,8 @@
  * Larry Finger 
  *
  
**/
-#ifndef _IO_H_
-#define _IO_H_
+#ifndef _RTL871X_IO_H_
+#define _RTL871X_IO_H_
 
 #include "osdep_service.h"
 #include "osdep_intf.h"
@@ -234,5 +234,4 @@ void r8712_write_port(struct _adapter *adapter, u32 addr, 
u32 cnt, u8 *pmem);
 uint r8712_alloc_io_queue(struct _adapter *adapter);
 void r8712_free_io_queue(struct _adapter *adapter);
 
-#endif /*_RTL8711_IO_H_*/
-
+#endif /*_RTL871X_IO_H_*/
diff --git a/drivers/staging/rtl8712/rtl871x_rf.h 
b/drivers/staging/rtl8712/rtl871x_rf.h
index cc54453cd424..7d98921a48fa 100644
--- a/drivers/staging/rtl8712/rtl871x_rf.h
+++ b/drivers/staging/rtl8712/rtl871x_rf.h
@@ -52,5 +52,4 @@ enum {
RTL8712_RFC_2T2R = 0x22
 };
 
-#endif /*_RTL8711_RF_H_*/
-
+#endif /*__RTL871X_RF_H_*/
-- 
2.20.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: erofs: fix an error handling in erofs_readdir()

2019-08-18 Thread Gao Xiang
On Sun, Aug 18, 2019 at 09:17:52PM +0800, kbuild test robot wrote:
> Hi Gao,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on linus/master]
> [cannot apply to v5.3-rc4 next-20190816]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]

... those patches should be applied to staging tree
since linux-next has not been updated yet...

Thanks,
Gao Xiang

> 
> url:
> https://github.com/0day-ci/linux/commits/Gao-Xiang/staging-erofs-fix-an-error-handling-in-erofs_readdir/20190818-191344
> config: arm64-allyesconfig (attached as .config)
> compiler: aarch64-linux-gcc (GCC) 7.4.0
> reproduce:
> wget 
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> GCC_VERSION=7.4.0 make.cross ARCH=arm64 
> 
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot 
> 
> All errors (new ones prefixed by >>):
> 
>drivers/staging/erofs/dir.c: In function 'erofs_readdir':
> >> drivers/staging/erofs/dir.c:110:11: error: 'EFSCORRUPTED' undeclared 
> >> (first use in this function); did you mean 'FS_NRSUPER'?
>err = -EFSCORRUPTED;
>   ^~~~
>   FS_NRSUPER
>drivers/staging/erofs/dir.c:110:11: note: each undeclared identifier is 
> reported only once for each function it appears in
> 
> vim +110 drivers/staging/erofs/dir.c
> 
> 85
> 86static int erofs_readdir(struct file *f, struct dir_context 
> *ctx)
> 87{
> 88struct inode *dir = file_inode(f);
> 89struct address_space *mapping = dir->i_mapping;
> 90const size_t dirsize = i_size_read(dir);
> 91unsigned int i = ctx->pos / EROFS_BLKSIZ;
> 92unsigned int ofs = ctx->pos % EROFS_BLKSIZ;
> 93int err = 0;
> 94bool initial = true;
> 95
> 96while (ctx->pos < dirsize) {
> 97struct page *dentry_page;
> 98struct erofs_dirent *de;
> 99unsigned int nameoff, maxsize;
>100
>101dentry_page = read_mapping_page(mapping, i, 
> NULL);
>102if (dentry_page == ERR_PTR(-ENOMEM)) {
>103errln("no memory to readdir of logical 
> block %u of nid %llu",
>104  i, EROFS_V(dir)->nid);
>105err = -ENOMEM;
>106break;
>107} else if (IS_ERR(dentry_page)) {
>108errln("fail to readdir of logical block 
> %u of nid %llu",
>109  i, EROFS_V(dir)->nid);
>  > 110err = -EFSCORRUPTED;
>111break;
>112}
>113
>114de = (struct erofs_dirent *)kmap(dentry_page);
>115
>116nameoff = le16_to_cpu(de->nameoff);
>117
>118if (unlikely(nameoff < sizeof(struct 
> erofs_dirent) ||
>119 nameoff >= PAGE_SIZE)) {
>120errln("%s, invalid de[0].nameoff %u",
>121  __func__, nameoff);
>122
>123err = -EIO;
>124goto skip_this;
>125}
>126
>127maxsize = min_t(unsigned int,
>128dirsize - ctx->pos + ofs, 
> PAGE_SIZE);
>129
>130/* search dirents at the arbitrary position */
>131if (unlikely(initial)) {
>132initial = false;
>133
>134ofs = roundup(ofs, sizeof(struct 
> erofs_dirent));
>135if (unlikely(ofs >= nameoff))
>136goto skip_this;
>137}
>138
>139err = erofs_fill_dentries(ctx, de, , 
> nameoff, maxsize);
>140skip_this:
>14

Re: [PATCH] staging: erofs: fix an error handling in erofs_readdir()

2019-08-18 Thread kbuild test robot
Hi Gao,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3-rc4 next-20190816]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Gao-Xiang/staging-erofs-fix-an-error-handling-in-erofs_readdir/20190818-191344
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 7.4.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot 

All errors (new ones prefixed by >>):

   drivers/staging/erofs/dir.c: In function 'erofs_readdir':
>> drivers/staging/erofs/dir.c:110:11: error: 'EFSCORRUPTED' undeclared (first 
>> use in this function); did you mean 'FS_NRSUPER'?
   err = -EFSCORRUPTED;
  ^~~~
  FS_NRSUPER
   drivers/staging/erofs/dir.c:110:11: note: each undeclared identifier is 
reported only once for each function it appears in

vim +110 drivers/staging/erofs/dir.c

85  
86  static int erofs_readdir(struct file *f, struct dir_context *ctx)
87  {
88  struct inode *dir = file_inode(f);
89  struct address_space *mapping = dir->i_mapping;
90  const size_t dirsize = i_size_read(dir);
91  unsigned int i = ctx->pos / EROFS_BLKSIZ;
92  unsigned int ofs = ctx->pos % EROFS_BLKSIZ;
93  int err = 0;
94  bool initial = true;
95  
96  while (ctx->pos < dirsize) {
97  struct page *dentry_page;
98  struct erofs_dirent *de;
99  unsigned int nameoff, maxsize;
   100  
   101  dentry_page = read_mapping_page(mapping, i, NULL);
   102  if (dentry_page == ERR_PTR(-ENOMEM)) {
   103  errln("no memory to readdir of logical block %u 
of nid %llu",
   104i, EROFS_V(dir)->nid);
   105  err = -ENOMEM;
   106  break;
   107  } else if (IS_ERR(dentry_page)) {
   108  errln("fail to readdir of logical block %u of 
nid %llu",
   109i, EROFS_V(dir)->nid);
 > 110  err = -EFSCORRUPTED;
   111  break;
   112  }
   113  
   114  de = (struct erofs_dirent *)kmap(dentry_page);
   115  
   116  nameoff = le16_to_cpu(de->nameoff);
   117  
   118  if (unlikely(nameoff < sizeof(struct erofs_dirent) ||
   119   nameoff >= PAGE_SIZE)) {
   120  errln("%s, invalid de[0].nameoff %u",
   121__func__, nameoff);
   122  
   123  err = -EIO;
   124  goto skip_this;
   125  }
   126  
   127  maxsize = min_t(unsigned int,
   128  dirsize - ctx->pos + ofs, PAGE_SIZE);
   129  
   130  /* search dirents at the arbitrary position */
   131  if (unlikely(initial)) {
   132  initial = false;
   133  
   134  ofs = roundup(ofs, sizeof(struct erofs_dirent));
   135  if (unlikely(ofs >= nameoff))
   136  goto skip_this;
   137  }
   138  
   139  err = erofs_fill_dentries(ctx, de, , nameoff, 
maxsize);
   140  skip_this:
   141  kunmap(dentry_page);
   142  
   143  put_page(dentry_page);
   144  
   145  ctx->pos = blknr_to_addr(i) + ofs;
   146  
   147  if (unlikely(err))
   148  break;
   149  ++i;
   150  ofs = 0;
   151  }
   152  return err < 0 ? err : 0;
   153  }
   154  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: erofs: refuse to mount images with malformed volume name

2019-08-18 Thread Chao Yu
On 2019-8-18 18:28, Gao Xiang wrote:
> From: Gao Xiang 
> 
> As Richard reminder [1], A valid volume name should be
> ended in NIL terminator within the length of volume_name.
> 
> Since this field currently isn't really used, let's fix
> it to avoid potential bugs in the future.
> 
> [1] 
> https://lore.kernel.org/r/1133002215.69049.1566119033047.javamail.zim...@nod.at/
> 
> Reported-by: Richard Weinberger 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


[PATCH v4] staging: erofs: fix an error handling in erofs_readdir()

2019-08-18 Thread Gao Xiang
From: Gao Xiang 

Richard observed a forever loop of erofs_read_raw_page() [1]
which can be generated by forcely setting ->u.i_blkaddr
to 0xdeadbeef (as my understanding block layer can
handle access beyond end of device correctly).

After digging into that, it seems the problem is highly
related with directories and then I found the root cause
is an improper error handling in erofs_readdir().

Let's fix it now.

[1] 
https://lore.kernel.org/r/1163995781.68824.1566084358245.javamail.zim...@nod.at/

Reported-by: Richard Weinberger 
Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations")
Cc:  # 4.19+
Reviewed-by: Chao Yu 
Signed-off-by: Gao Xiang 
---
changelog from v3:
 - kill message when memory allocation fails as suggested by Matthew;

[RESEND] --> add the missing v3 version in subject, no logic change.

changelog from v2:
 - transform EIO to EFSCORRUPTED as suggested by Matthew;

changelog from v1:
 - fix the incorrect external link in commit message.

This patch is based on staging-testing tree and
https://lore.kernel.org/r/20190817082313.21040-1-hsiang...@aol.com/
can still be properly applied after this patch.

 drivers/staging/erofs/dir.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/erofs/dir.c b/drivers/staging/erofs/dir.c
index 5f38382637e6..77ef856df9f3 100644
--- a/drivers/staging/erofs/dir.c
+++ b/drivers/staging/erofs/dir.c
@@ -82,8 +82,15 @@ static int erofs_readdir(struct file *f, struct dir_context 
*ctx)
unsigned int nameoff, maxsize;
 
dentry_page = read_mapping_page(mapping, i, NULL);
-   if (IS_ERR(dentry_page))
-   continue;
+   if (dentry_page == ERR_PTR(-ENOMEM)) {
+   err = -ENOMEM;
+   break;
+   } else if (IS_ERR(dentry_page)) {
+   errln("fail to readdir of logical block %u of nid %llu",
+ i, EROFS_V(dir)->nid);
+   err = -EFSCORRUPTED;
+   break;
+   }
 
de = (struct erofs_dirent *)kmap(dentry_page);
 
-- 
2.17.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()

2019-08-18 Thread Gao Xiang
On Sun, Aug 18, 2019 at 05:33:14AM -0700, Matthew Wilcox wrote:
> On Sun, Aug 18, 2019 at 11:21:11AM +0800, Gao Xiang wrote:
> > +   if (dentry_page == ERR_PTR(-ENOMEM)) {
> > +   errln("no memory to readdir of logical block %u of nid 
> > %llu",
> > + i, EROFS_V(dir)->nid);
> 
> I don't think you need the error message.  If we get a memory allocation
> failure, there's already going to be a lot of spew in the logs from the
> mm system.  And if we do fail to allocate memory, we don't need to know
> the logical block number or the nid -- it has nothiing to do with those;
> the system simply ran out of memory.

OK, I agree with you. There is a messy of messages when
memory allocation fail.

Since I don't really care apart from crashing or hanging
the kernel, I will resend the patch to make you and Chao
happy... :)

Thanks,
Gao Xiang

> 
> > +   err = -ENOMEM;
> > +   break;
> > +   } else if (IS_ERR(dentry_page)) {
> > +   errln("fail to readdir of logical block %u of nid %llu",
> > + i, EROFS_V(dir)->nid);
> > +   err = -EFSCORRUPTED;
> > +   break;
> > +   }
> >  
> > de = (struct erofs_dirent *)kmap(dentry_page);
> >  
> > -- 
> > 2.17.1
> > 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()

2019-08-18 Thread Matthew Wilcox
On Sun, Aug 18, 2019 at 11:21:11AM +0800, Gao Xiang wrote:
> + if (dentry_page == ERR_PTR(-ENOMEM)) {
> + errln("no memory to readdir of logical block %u of nid 
> %llu",
> +   i, EROFS_V(dir)->nid);

I don't think you need the error message.  If we get a memory allocation
failure, there's already going to be a lot of spew in the logs from the
mm system.  And if we do fail to allocate memory, we don't need to know
the logical block number or the nid -- it has nothiing to do with those;
the system simply ran out of memory.

> + err = -ENOMEM;
> + break;
> + } else if (IS_ERR(dentry_page)) {
> + errln("fail to readdir of logical block %u of nid %llu",
> +   i, EROFS_V(dir)->nid);
> + err = -EFSCORRUPTED;
> + break;
> + }
>  
>   de = (struct erofs_dirent *)kmap(dentry_page);
>  
> -- 
> 2.17.1
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()

2019-08-18 Thread Chao Yu
On 2019-8-18 11:21, Gao Xiang wrote:
> From: Gao Xiang 
> 
> Richard observed a forever loop of erofs_read_raw_page() [1]
> which can be generated by forcely setting ->u.i_blkaddr
> to 0xdeadbeef (as my understanding block layer can
> handle access beyond end of device correctly).
> 
> After digging into that, it seems the problem is highly
> related with directories and then I found the root cause
> is an improper error handling in erofs_readdir().
> 
> Let's fix it now.
> 
> [1] 
> https://lore.kernel.org/r/1163995781.68824.1566084358245.javamail.zim...@nod.at/
> 
> Reported-by: Richard Weinberger 
> Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations")
> Cc:  # 4.19+
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2] staging: erofs: fix an error handling in erofs_readdir()

2019-08-18 Thread Chao Yu
Hi Xiang,

On 2019-8-18 18:52, Gao Xiang wrote:
> Hi Chao,
> 
> On Sun, Aug 18, 2019 at 06:39:52PM +0800, Chao Yu wrote:
>> On 2019-8-18 10:53, Matthew Wilcox wrote:
>>> On Sun, Aug 18, 2019 at 10:32:45AM +0800, Gao Xiang wrote:
 On Sat, Aug 17, 2019 at 07:20:55PM -0700, Matthew Wilcox wrote:
> On Sun, Aug 18, 2019 at 09:56:31AM +0800, Gao Xiang wrote:
>> @@ -82,8 +82,12 @@ static int erofs_readdir(struct file *f, struct 
>> dir_context *ctx)
>>  unsigned int nameoff, maxsize;
>>  
>>  dentry_page = read_mapping_page(mapping, i, NULL);
>> -if (IS_ERR(dentry_page))
>> -continue;
>> +if (IS_ERR(dentry_page)) {
>> +errln("fail to readdir of logical block %u of 
>> nid %llu",
>> +  i, EROFS_V(dir)->nid);
>> +err = PTR_ERR(dentry_page);
>> +break;
>
> I don't think you want to use the errno that came back from
> read_mapping_page() (which is, I think, always going to be -EIO).
> Rather you want -EFSCORRUPTED, at least if I understand the recent
> patches to ext2/ext4/f2fs/xfs/...

 Thanks for your reply and noticing this. :)

 Yes, as I talked with you about read_mapping_page() in a xfs related
 topic earlier, I think I fully understand what returns here.

 I actually had some concern about that before sending out this patch.
 You know the status is
PG_uptodate is not set and PG_error is set here.

 But we cannot know it is actually a disk read error or due to
 corrupted images (due to lack of page flags or some status, and
 I think it could be a waste of page structure space for such
 corrupted image or disk error)...

 And some people also like propagate errors from insiders...
 (and they could argue about err = -EFSCORRUPTED as well..)

 I'd like hear your suggestion about this after my words above?
 still return -EFSCORRUPTED?
>>>
>>> I don't think it matters whether it's due to a disk error or a corrupted
>>> image.  We can't read the directory entry, so we should probably return
>>> -EFSCORRUPTED.  Thinking about it some more, read_mapping_page() can
>>> also return -ENOMEM, so it should probably look something like this:
>>>
>>> err = 0;
>>> if (dentry_page == ERR_PTR(-ENOMEM))
>>> err = -ENOMEM;
>>> else if (IS_ERR(dentry_page)) {
>>> errln("fail to readdir of logical block %u of nid %llu",
>>>   i, EROFS_V(dir)->nid);
>>> err = -EFSCORRUPTED;
>>
>> Well, if there is real IO error happen under filesystem, we should return 
>> -EIO
>> instead of EFSCORRUPTED?
>>
>> The right fix may be that doing sanity check on on-disk blkaddr, and return
>> -EFSCORRUPTED if the blkaddr is invalid and propagate the error to its caller
>> erofs_readdir(), IIUC below error info.
> 
> In my thought, I actually don't care what is actually returned
> (In other words, I have no tendency about EFSCORRUPTED / EIO)
> as long as it behaves normal for corrupted image.

I doubt that user can and be willing to handle different error code in there
error path.

> 
> A little concern is that I have no idea whether all user problems
> can handle EUCLEAN properly.

Yes, I can see it's widely used in fileystem, I guess it would be better to
update manual of common fs interfaces to let user be aware of the meaning of it.

> 
> I don't want to limit blkaddr as what ->blocks recorded in
> erofs_super_block since it's already used for our hotpatching
> approach and that field is only used for statfs() for users
> to know its visible size, and block layer will block such
> invalid block access.
> 
> All in all, that is minor. I think we can fix as what Matthew said.

Yeah, as we discuss offline, we need keep flexibility on current version for
android, and maybe we can add a feature to check blkaddr validation later.

Thanks,

> 
> Thanks,
> Gao Xiang
> 
>>
>>> [36297.354090] attempt to access beyond end of device
>>> [36297.354098] loop17: rw=0, want=29887428984, limit=1953128
>>> [36297.354107] attempt to access beyond end of device
>>> [36297.354109] loop17: rw=0, want=29887428480, limit=1953128
>>> [36301.827234] attempt to access beyond end of device
>>> [36301.827243] loop17: rw=0, want=29887428480, limit=1953128
>>
>> Thanks,
>>
>>> }
>>>
>>> if (err)
>>> break;
>>>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: erofs: refuse to mount images with malformed volume name

2019-08-18 Thread kbuild test robot
Hi Gao,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3-rc4 next-20190816]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Gao-Xiang/staging-erofs-refuse-to-mount-images-with-malformed-volume-name/20190818-193037
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 7.4.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=sparc64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot 

All errors (new ones prefixed by >>):

   drivers/staging/erofs/super.c: In function 'superblock_read':
>> drivers/staging/erofs/super.c:153:10: error: 'EFSCORRUPTED' undeclared 
>> (first use in this function); did you mean 'FS_NRSUPER'?
  ret = -EFSCORRUPTED;
 ^~~~
 FS_NRSUPER
   drivers/staging/erofs/super.c:153:10: note: each undeclared identifier is 
reported only once for each function it appears in

vim +153 drivers/staging/erofs/super.c

   140  
   141  sbi->root_nid = le16_to_cpu(layout->root_nid);
   142  sbi->inos = le64_to_cpu(layout->inos);
   143  
   144  sbi->build_time = le64_to_cpu(layout->build_time);
   145  sbi->build_time_nsec = le32_to_cpu(layout->build_time_nsec);
   146  
   147  memcpy(>s_uuid, layout->uuid, sizeof(layout->uuid));
   148  
   149  ret = strscpy(sbi->volume_name, layout->volume_name,
   150sizeof(layout->volume_name));
   151  if (ret < 0) {  /* -E2BIG */
   152  errln("bad volume name without NIL terminator");
 > 153  ret = -EFSCORRUPTED;
   154  goto out;
   155  }
   156  ret = 0;
   157  out:
   158  brelse(bh);
   159  return ret;
   160  }
   161  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: erofs: fix an error handling in erofs_readdir()

2019-08-18 Thread kbuild test robot
Hi Gao,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3-rc4 next-20190816]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Gao-Xiang/staging-erofs-fix-an-error-handling-in-erofs_readdir/20190818-191344
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 7.4.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=sparc64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot 

All errors (new ones prefixed by >>):

   drivers/staging//erofs/dir.c: In function 'erofs_readdir':
>> drivers/staging//erofs/dir.c:110:11: error: 'EFSCORRUPTED' undeclared (first 
>> use in this function)
   err = -EFSCORRUPTED;
  ^~~~
   drivers/staging//erofs/dir.c:110:11: note: each undeclared identifier is 
reported only once for each function it appears in

vim +/EFSCORRUPTED +110 drivers/staging//erofs/dir.c

85  
86  static int erofs_readdir(struct file *f, struct dir_context *ctx)
87  {
88  struct inode *dir = file_inode(f);
89  struct address_space *mapping = dir->i_mapping;
90  const size_t dirsize = i_size_read(dir);
91  unsigned int i = ctx->pos / EROFS_BLKSIZ;
92  unsigned int ofs = ctx->pos % EROFS_BLKSIZ;
93  int err = 0;
94  bool initial = true;
95  
96  while (ctx->pos < dirsize) {
97  struct page *dentry_page;
98  struct erofs_dirent *de;
99  unsigned int nameoff, maxsize;
   100  
   101  dentry_page = read_mapping_page(mapping, i, NULL);
   102  if (dentry_page == ERR_PTR(-ENOMEM)) {
   103  errln("no memory to readdir of logical block %u 
of nid %llu",
   104i, EROFS_V(dir)->nid);
   105  err = -ENOMEM;
   106  break;
   107  } else if (IS_ERR(dentry_page)) {
   108  errln("fail to readdir of logical block %u of 
nid %llu",
   109i, EROFS_V(dir)->nid);
 > 110  err = -EFSCORRUPTED;
   111  break;
   112  }
   113  
   114  de = (struct erofs_dirent *)kmap(dentry_page);
   115  
   116  nameoff = le16_to_cpu(de->nameoff);
   117  
   118  if (unlikely(nameoff < sizeof(struct erofs_dirent) ||
   119   nameoff >= PAGE_SIZE)) {
   120  errln("%s, invalid de[0].nameoff %u",
   121__func__, nameoff);
   122  
   123  err = -EIO;
   124  goto skip_this;
   125  }
   126  
   127  maxsize = min_t(unsigned int,
   128  dirsize - ctx->pos + ofs, PAGE_SIZE);
   129  
   130  /* search dirents at the arbitrary position */
   131  if (unlikely(initial)) {
   132  initial = false;
   133  
   134  ofs = roundup(ofs, sizeof(struct erofs_dirent));
   135  if (unlikely(ofs >= nameoff))
   136  goto skip_this;
   137  }
   138  
   139  err = erofs_fill_dentries(ctx, de, , nameoff, 
maxsize);
   140  skip_this:
   141  kunmap(dentry_page);
   142  
   143  put_page(dentry_page);
   144  
   145  ctx->pos = blknr_to_addr(i) + ofs;
   146  
   147  if (unlikely(err))
   148  break;
   149  ++i;
   150  ofs = 0;
   151  }
   152  return err < 0 ? err : 0;
   153  }
   154  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2] staging: erofs: fix an error handling in erofs_readdir()

2019-08-18 Thread Gao Xiang
Hi Chao,

On Sun, Aug 18, 2019 at 06:39:52PM +0800, Chao Yu wrote:
> On 2019-8-18 10:53, Matthew Wilcox wrote:
> > On Sun, Aug 18, 2019 at 10:32:45AM +0800, Gao Xiang wrote:
> >> On Sat, Aug 17, 2019 at 07:20:55PM -0700, Matthew Wilcox wrote:
> >>> On Sun, Aug 18, 2019 at 09:56:31AM +0800, Gao Xiang wrote:
>  @@ -82,8 +82,12 @@ static int erofs_readdir(struct file *f, struct 
>  dir_context *ctx)
>   unsigned int nameoff, maxsize;
>   
>   dentry_page = read_mapping_page(mapping, i, NULL);
>  -if (IS_ERR(dentry_page))
>  -continue;
>  +if (IS_ERR(dentry_page)) {
>  +errln("fail to readdir of logical block %u of 
>  nid %llu",
>  +  i, EROFS_V(dir)->nid);
>  +err = PTR_ERR(dentry_page);
>  +break;
> >>>
> >>> I don't think you want to use the errno that came back from
> >>> read_mapping_page() (which is, I think, always going to be -EIO).
> >>> Rather you want -EFSCORRUPTED, at least if I understand the recent
> >>> patches to ext2/ext4/f2fs/xfs/...
> >>
> >> Thanks for your reply and noticing this. :)
> >>
> >> Yes, as I talked with you about read_mapping_page() in a xfs related
> >> topic earlier, I think I fully understand what returns here.
> >>
> >> I actually had some concern about that before sending out this patch.
> >> You know the status is
> >>PG_uptodate is not set and PG_error is set here.
> >>
> >> But we cannot know it is actually a disk read error or due to
> >> corrupted images (due to lack of page flags or some status, and
> >> I think it could be a waste of page structure space for such
> >> corrupted image or disk error)...
> >>
> >> And some people also like propagate errors from insiders...
> >> (and they could argue about err = -EFSCORRUPTED as well..)
> >>
> >> I'd like hear your suggestion about this after my words above?
> >> still return -EFSCORRUPTED?
> > 
> > I don't think it matters whether it's due to a disk error or a corrupted
> > image.  We can't read the directory entry, so we should probably return
> > -EFSCORRUPTED.  Thinking about it some more, read_mapping_page() can
> > also return -ENOMEM, so it should probably look something like this:
> > 
> > err = 0;
> > if (dentry_page == ERR_PTR(-ENOMEM))
> > err = -ENOMEM;
> > else if (IS_ERR(dentry_page)) {
> > errln("fail to readdir of logical block %u of nid %llu",
> >   i, EROFS_V(dir)->nid);
> > err = -EFSCORRUPTED;
> 
> Well, if there is real IO error happen under filesystem, we should return -EIO
> instead of EFSCORRUPTED?
> 
> The right fix may be that doing sanity check on on-disk blkaddr, and return
> -EFSCORRUPTED if the blkaddr is invalid and propagate the error to its caller
> erofs_readdir(), IIUC below error info.

In my thought, I actually don't care what is actually returned
(In other words, I have no tendency about EFSCORRUPTED / EIO)
as long as it behaves normal for corrupted image.

A little concern is that I have no idea whether all user problems
can handle EUCLEAN properly.

I don't want to limit blkaddr as what ->blocks recorded in
erofs_super_block since it's already used for our hotpatching
approach and that field is only used for statfs() for users
to know its visible size, and block layer will block such
invalid block access.

All in all, that is minor. I think we can fix as what Matthew said.

Thanks,
Gao Xiang

> 
> > [36297.354090] attempt to access beyond end of device
> > [36297.354098] loop17: rw=0, want=29887428984, limit=1953128
> > [36297.354107] attempt to access beyond end of device
> > [36297.354109] loop17: rw=0, want=29887428480, limit=1953128
> > [36301.827234] attempt to access beyond end of device
> > [36301.827243] loop17: rw=0, want=29887428480, limit=1953128
> 
> Thanks,
> 
> > }
> > 
> > if (err)
> > break;
> > 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2] staging: erofs: fix an error handling in erofs_readdir()

2019-08-18 Thread Chao Yu
On 2019-8-18 10:53, Matthew Wilcox wrote:
> On Sun, Aug 18, 2019 at 10:32:45AM +0800, Gao Xiang wrote:
>> On Sat, Aug 17, 2019 at 07:20:55PM -0700, Matthew Wilcox wrote:
>>> On Sun, Aug 18, 2019 at 09:56:31AM +0800, Gao Xiang wrote:
 @@ -82,8 +82,12 @@ static int erofs_readdir(struct file *f, struct 
 dir_context *ctx)
unsigned int nameoff, maxsize;
  
dentry_page = read_mapping_page(mapping, i, NULL);
 -  if (IS_ERR(dentry_page))
 -  continue;
 +  if (IS_ERR(dentry_page)) {
 +  errln("fail to readdir of logical block %u of nid %llu",
 +i, EROFS_V(dir)->nid);
 +  err = PTR_ERR(dentry_page);
 +  break;
>>>
>>> I don't think you want to use the errno that came back from
>>> read_mapping_page() (which is, I think, always going to be -EIO).
>>> Rather you want -EFSCORRUPTED, at least if I understand the recent
>>> patches to ext2/ext4/f2fs/xfs/...
>>
>> Thanks for your reply and noticing this. :)
>>
>> Yes, as I talked with you about read_mapping_page() in a xfs related
>> topic earlier, I think I fully understand what returns here.
>>
>> I actually had some concern about that before sending out this patch.
>> You know the status is
>>PG_uptodate is not set and PG_error is set here.
>>
>> But we cannot know it is actually a disk read error or due to
>> corrupted images (due to lack of page flags or some status, and
>> I think it could be a waste of page structure space for such
>> corrupted image or disk error)...
>>
>> And some people also like propagate errors from insiders...
>> (and they could argue about err = -EFSCORRUPTED as well..)
>>
>> I'd like hear your suggestion about this after my words above?
>> still return -EFSCORRUPTED?
> 
> I don't think it matters whether it's due to a disk error or a corrupted
> image.  We can't read the directory entry, so we should probably return
> -EFSCORRUPTED.  Thinking about it some more, read_mapping_page() can
> also return -ENOMEM, so it should probably look something like this:
> 
>   err = 0;
>   if (dentry_page == ERR_PTR(-ENOMEM))
>   err = -ENOMEM;
>   else if (IS_ERR(dentry_page)) {
>   errln("fail to readdir of logical block %u of nid %llu",
> i, EROFS_V(dir)->nid);
>   err = -EFSCORRUPTED;

Well, if there is real IO error happen under filesystem, we should return -EIO
instead of EFSCORRUPTED?

The right fix may be that doing sanity check on on-disk blkaddr, and return
-EFSCORRUPTED if the blkaddr is invalid and propagate the error to its caller
erofs_readdir(), IIUC below error info.

> [36297.354090] attempt to access beyond end of device
> [36297.354098] loop17: rw=0, want=29887428984, limit=1953128
> [36297.354107] attempt to access beyond end of device
> [36297.354109] loop17: rw=0, want=29887428480, limit=1953128
> [36301.827234] attempt to access beyond end of device
> [36301.827243] loop17: rw=0, want=29887428480, limit=1953128

Thanks,

>   }
> 
>   if (err)
>   break;
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


[PATCH] staging: erofs: refuse to mount images with malformed volume name

2019-08-18 Thread Gao Xiang
From: Gao Xiang 

As Richard reminder [1], A valid volume name should be
ended in NIL terminator within the length of volume_name.

Since this field currently isn't really used, let's fix
it to avoid potential bugs in the future.

[1] 
https://lore.kernel.org/r/1133002215.69049.1566119033047.javamail.zim...@nod.at/

Reported-by: Richard Weinberger 
Signed-off-by: Gao Xiang 
---
 drivers/staging/erofs/super.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index f65a1ff9f42f..2da471010a86 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -131,9 +131,14 @@ static int superblock_read(struct super_block *sb)
sbi->build_time_nsec = le32_to_cpu(layout->build_time_nsec);
 
memcpy(>s_uuid, layout->uuid, sizeof(layout->uuid));
-   memcpy(sbi->volume_name, layout->volume_name,
-  sizeof(layout->volume_name));
 
+   ret = strscpy(sbi->volume_name, layout->volume_name,
+ sizeof(layout->volume_name));
+   if (ret < 0) {  /* -E2BIG */
+   errln("bad volume name without NIL terminator");
+   ret = -EFSCORRUPTED;
+   goto out;
+   }
ret = 0;
 out:
brelse(bh);
-- 
2.17.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Chao Yu
Hi Richard,

On 2019-8-18 17:21, Richard Weinberger wrote:
> For normal use I see no problem at all.
> I fear distros that try to mount anything you plug into your USB.
> 
> At least SUSE already blacklists erofs:
> https://github.com/openSUSE/suse-module-tools/blob/master/suse-module-tools.spec#L24

Thanks for letting us know current status of erofs in SUSE distro.

Currently erofs cares more about the requirement of Android, in there, we are
safe on fuzzed image case as dm-verity can keep all partition data being
verified before mount.

For other scenarios, like distro, erofs should improve itself step by step as
many mainline filesystems in many aspects to fit the there-in requirement. :)

Thanks,

> 
> Thanks,
> //richard
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation

2019-08-18 Thread Chuanhong Guo
Hi!

On Sun, Aug 18, 2019 at 5:51 PM Oleksij Rempel  wrote:
>
> lets see more code:
> drivers/staging/mt7621-mmc/sd.c
> /* clock source for host: global */
> #if defined(CONFIG_SOC_MT7620)
> static u32 hclks[] = {4800}; /* +/- by chhung */
> #elif defined(CONFIG_SOC_MT7621)
> static u32 hclks[] = {5000}; /* +/- by chhung */
> #endif
>
> hm.. 50Mhz again. Feels like APB clock.
>
> ./drivers/staging/mt7621-dts/mt7621.dtsi
> cpuclock: cpuclock@0 {
> #clock-cells = <0>;
> compatible = "fixed-clock";
>
> /* FIXME: there should be way to detect this */
> clock-frequency = <88000>;
> };
>
> Your driver is trying to cover cpuclock
>
> sysclock: sysclock@0 {
> #clock-cells = <0>;
> compatible = "fixed-clock";
>
> /* This is normally 1/4 of cpuclock */
> clock-frequency = <22000>;
> };
>
> and most probably system clock alias "bus clock", most probably AHB.

This sysclock was the 50MHz clock in OpenWrt. It's set as "bus clock"
upstream by an incorrect commit.
As already stated in previous reply: I'm not going to make assumption
about clock plan using OpenWrt's device tree because it already
contains several mistakes on clocks. Since the upstream device tree
comes from there, I don't trust it either. You might want to check out
patch 6/6 in this series where the original author of this commit in
openwrt fixed some clocks and I ported it here.

> [...]
> > This debate goes nowhere. I've clarified the situation and made my
> > point. Of course I can't read through the ancient and heavily hacked
> > vendor kernel to figure out a clock plan myself.
>
> Ok, I provided you some productive technical hints how it should be
> done. I don't think mt7620 has better documentation then mt7621 and even
> in this case it was possible to make more or less good driver.

It does. A clock plan for mt7620 is available in MT7620 Programming
Guide, Page 14.

Regards,
Chuanhong Guo
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation

2019-08-18 Thread Oleksij Rempel
Am 18.08.19 um 10:44 schrieb Chuanhong Guo:
> On Sun, Aug 18, 2019 at 4:26 PM Chuanhong Guo  wrote:
>>
>> Hi!
>>
>> On Sun, Aug 18, 2019 at 3:59 PM Oleksij Rempel  
>> wrote:
>>>
>>> Am 18.08.19 um 09:19 schrieb Chuanhong Guo:
 Hi!

 On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel  
 wrote:
>
>>> We have at least 2 know registers:
>>> SYSC_REG_CPLL_CLKCFG0 - it provides some information about boostrapped
>>> refclock. PLL and dividers used for CPU and some sort of BUS (AHB?).
>>> SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for
>>> all or some ip cores.
>>> What is probably missing is a set of dividers for
>>> each ip core. From your words it is not document.
>>
>> The specific missing part I was referring to, is parent clocks for
>> every gates. I'm not going to assume this with current openwrt device
>> tree because some peripherals doesn't have a clock binding at all or
>> have a dummy one there.
>
> Ok, then I do not understand what is the motivation to upstream
> something what is not nearly ready for use.

 Why isn't it "ready for use" then?
 A complete mt7621-pll driver will contain two parts:
 1. A clock provider which outputs several clocks
 2. A clock gate with parent clocks properly configured

 Two clocks provided here are just two clocks that can't be controlled
 in kernel no matter where it goes (arch/mips/ralink or drivers/clk).
 Having a working CPU clock provider is better than defining a fixed
 clock in dts because CPU clock can be controlled by bootloader.
 (BTW description for CPU PLL register is also missing in datasheet.)
 Clock gate is an unrelated part and there is no information to
 properly implement it unless MTK decided to release a clock plan
 somehow.
>>>
>>> With other words, your complete system is running with unknown clock
>>> rates.
>>
>> And without this patchset the complete system is running with unknown
>> clock and, even worse, we make assumptions about what clock bootloader
>> uses, hardcoded it in dts and hope it is the correct value.
>>
>>> The source clock in the clock three can be configured differently
>>> by bootloader but you don't know how it is done how and it is not
>>> documented.
>>
>> Actually, I don't know about this and I didn't wrote the original
>> clock calculation code. I just ported it from downstream OpenWrt
>> kernel. Here's a piece of code from Mediatek's SDK kernel:
>>
>> case 0:
>> reg = (*(volatile u32 *)(RALINK_SYSCTL_BASE + 0x44));
>> cpu_fdiv = ((reg >> 8) & 0x1F);
>> cpu_ffrac = (reg & 0x1F);
>> mips_cpu_feq = (500 * cpu_ffrac / cpu_fdiv) * 1000 * 1000;
>> break;
>> case 1: //CPU PLL
>> reg = (*(volatile u32 *)(RALINK_MEMCTRL_BASE + 0x648));
>> fbdiv = ((reg >> 4) & 0x7F) + 1;
>> reg = (*(volatile u32 *)(RALINK_SYSCTL_BASE + 0x10));
>> reg = (reg >> 6) & 0x7;
>> if(reg >= 6) { //25Mhz Xtal
>> mips_cpu_feq = 25 * fbdiv * 1000 * 1000;
>> } else if(reg >=3) { //40Mhz Xtal
>> mips_cpu_feq = 20 * fbdiv * 1000 * 1000;
>> } else { // 20Mhz Xtal
>> /* TODO */
>> }
>> break;
>>
>>
>>
>>>
> This code is currently on prototyping phase

 Code for clock calculation is done, not "prototyping".

> It means, we cannot expect that this driver will be fixed any time soon.

 I think clock gating is a separated feature instead of a broken part
 that has to be fixed.
>>>
>>> Ok, i would agree with it. But from what you said, this feature will be
>>> never implemented.
>>>
>>> So, I repeat my question. What is the point to upstream code for a
>>> system, which has not enough information to get proper clock rate even
>>> for uart? or is uart running with cpu or bus clock rate?
>>
>> uart runs of a fixed 50MHz clock according to another piece of code
>> from MTK SDK:
>> (a pastebin version here for better readability. This specific
>> question has nothing to do with patch reviewing and doesn't need to be
>> preserved in mail forever.)
>> https://paste.ubuntu.com/p/fYmtDFW9nh/

ok.

lets see more code:
drivers/staging/mt7621-mmc/sd.c
/* clock source for host: global */
#if defined(CONFIG_SOC_MT7620)
static u32 hclks[] = {4800}; /* +/- by chhung */
#elif defined(CONFIG_SOC_MT7621)
static u32 hclks[] = {5000}; /* +/- by chhung */
#endif

hm.. 50Mhz again. Feels like APB clock.

./drivers/staging/mt7621-dts/mt7621.dtsi
cpuclock: cpuclock@0 {
#clock-cells = <0>;
compatible = "fixed-clock";

/* FIXME: there should be way to detect this */
clock-frequency = <88000>;
};

Your driver is trying to cover cpuclock

sysclock: sysclock@0 {
#clock-cells = <0>;
compatible = "fixed-clock";

/* This is normally 1/4 of 

Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Gao Xiang
On Sun, Aug 18, 2019 at 11:03:53AM +0200, Richard Weinberger wrote:
> - Urspr??ngliche Mail -
> > I agree with you, but what can we do now is trying our best to fuzz
> > all the fields.
> > 
> > So, what is your opinion about EROFS?
> 
> All I'm saying is that you should not blindly trust the disk.

I completely agree with you, and I'm teaching EROFS to
make the little naughty boy more strong... (we already
have many error handling code, but I think I will teach him
more, yes.)

> 
> Another thing that raises my attention is in superblock_read():
> memcpy(sbi->volume_name, layout->volume_name,
>sizeof(layout->volume_name));
> 
> Where do you check whether ->volume_name has a NUL terminator?
> Currently this field has no user, maybe will add a check upon usage.
> But this kind of things makes me wonder.

Yes, I think this is a good point. :)
Since volume_name is not used currently, I will fix it when
it use late.

I will make a note here (or more straightforward, I will fix it
to avoid potential bug now.)

Thanks,
Gao Xiang

> 
> Thanks,
> //richard
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Richard Weinberger
- Ursprüngliche Mail -
> You have looked at reiserfs lately, right?  :)

Don't remind me of that. ;-)
 
> Not to say that erofs shouldn't be worked on to fix these kinds of
> issues, just that it's not an unheard of thing to trust the disk image.
> Especially for the normal usage model of erofs, where the whole disk
> image is verfied before it is allowed to be mounted as part of the boot
> process.

For normal use I see no problem at all.
I fear distros that try to mount anything you plug into your USB.

At least SUSE already blacklists erofs:
https://github.com/openSUSE/suse-module-tools/blob/master/suse-module-tools.spec#L24

Thanks,
//richard
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()y

2019-08-18 Thread Gao Xiang
On Sun, Aug 18, 2019 at 05:10:38PM +0800, Gao Xiang via Linux-erofs wrote:
> Hi Richard,
> 
> On Sun, Aug 18, 2019 at 10:33:33AM +0200, Richard Weinberger wrote:
> > - Urspr?ngliche Mail -
> > > changelog from v2:
> > > - transform EIO to EFSCORRUPTED as suggested by Matthew;
> > 
> > erofs does not define EFSCORRUPTED, so the build fails.
> > At least on Linus' tree as of today.
> 
> Thanks for your reply :)
> 
> I write all patches based on staging tree and do more cleanups further
> than Linus' tree, EFSCORRUPTED was already introduced by Pavel days before...

Sorry, I mean "introduced which was suggested by Paval"...

> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing=a6b9b1d5eae61a68085030e50d56265dec5baa94
> 
> which can be fetched from
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git -b 
> staging-next
> 
> Thanks,
> Gao Xiang
> 
> > 
> > Thanks,
> > //richard
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()

2019-08-18 Thread Gao Xiang
Hi Richard,

On Sun, Aug 18, 2019 at 10:33:33AM +0200, Richard Weinberger wrote:
> - Urspr?ngliche Mail -
> > changelog from v2:
> > - transform EIO to EFSCORRUPTED as suggested by Matthew;
> 
> erofs does not define EFSCORRUPTED, so the build fails.
> At least on Linus' tree as of today.

Thanks for your reply :)

I write all patches based on staging tree and do more cleanups further
than Linus' tree, EFSCORRUPTED was already introduced by Pavel days before...
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing=a6b9b1d5eae61a68085030e50d56265dec5baa94

which can be fetched from
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git -b staging-next

Thanks,
Gao Xiang

> 
> Thanks,
> //richard
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Greg Kroah-Hartman
On Sun, Aug 18, 2019 at 11:03:53AM +0200, Richard Weinberger wrote:
> - Ursprüngliche Mail -
> > I agree with you, but what can we do now is trying our best to fuzz
> > all the fields.
> > 
> > So, what is your opinion about EROFS?
> 
> All I'm saying is that you should not blindly trust the disk.
> 
> Another thing that raises my attention is in superblock_read():
> memcpy(sbi->volume_name, layout->volume_name,
>sizeof(layout->volume_name));
> 
> Where do you check whether ->volume_name has a NUL terminator?
> Currently this field has no user, maybe will add a check upon usage.
> But this kind of things makes me wonder.

You have looked at reiserfs lately, right?  :)

Not to say that erofs shouldn't be worked on to fix these kinds of
issues, just that it's not an unheard of thing to trust the disk image.
Especially for the normal usage model of erofs, where the whole disk
image is verfied before it is allowed to be mounted as part of the boot
process.

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Richard Weinberger
- Ursprüngliche Mail -
> I agree with you, but what can we do now is trying our best to fuzz
> all the fields.
> 
> So, what is your opinion about EROFS?

All I'm saying is that you should not blindly trust the disk.

Another thing that raises my attention is in superblock_read():
memcpy(sbi->volume_name, layout->volume_name,
   sizeof(layout->volume_name));

Where do you check whether ->volume_name has a NUL terminator?
Currently this field has no user, maybe will add a check upon usage.
But this kind of things makes me wonder.

Thanks,
//richard
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


[GIT PULL] Staging/IIO driver fixes for 5.3-rc5

2019-08-18 Thread Greg KH
The following changes since commit d45331b00ddb179e291766617259261c112db872:

  Linux 5.3-rc4 (2019-08-11 13:26:41 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 
tags/staging-5.3-rc5

for you to fetch changes up to 48b30e10bfc20ec6195642cc09ea6f08a8015df7:

  Merge tag 'iio-fixes-for-5.3b' of 
git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio into staging-linus 
(2019-08-12 22:47:59 +0200)


Staging/IIO fixes for 5.3-rc5

Here are 4 small staging and iio driver fixes for 5.3-rc5

Two are for the dt3000 comedi driver for some reported problems found in
that codebase, and two are some small iio fixes.

All of these have been in linux-next this week with no reported issues.

Signed-off-by: Greg Kroah-Hartman 


Greg Kroah-Hartman (1):
  Merge tag 'iio-fixes-for-5.3b' of git://git.kernel.org/.../jic23/iio into 
staging-linus

Ian Abbott (2):
  staging: comedi: dt3000: Fix signed integer overflow 'divider * base'
  staging: comedi: dt3000: Fix rounding up of timer divisor

Jacopo Mondi (1):
  iio: adc: max9611: Fix temperature reading in probe

Nuno Sá (1):
  iio: frequency: adf4371: Fix output frequency setting

 drivers/iio/adc/max9611.c   | 2 +-
 drivers/iio/frequency/adf4371.c | 8 
 drivers/staging/comedi/drivers/dt3000.c | 8 
 3 files changed, 9 insertions(+), 9 deletions(-)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Gao Xiang
On Sun, Aug 18, 2019 at 10:16:50AM +0200, Richard Weinberger wrote:
> - Urspr?ngliche Mail -
> >> While digging a little into the code I noticed that you have very few
> >> checks of the on-disk data.
> >> For example ->u.i_blkaddr. I gave it a try and created a
> >> malformed filesystem where u.i_blkaddr is 0xdeadbeef, it causes the kernel
> >> to loop forever around erofs_read_raw_page().
> > 
> > I don't fuzz all the on-disk fields for EROFS, I will do later..
> > You can see many in-kernel filesystems are still hardening the related
> > stuff. Anyway, I will dig into this field you mentioned recently, but
> > I think it can be fixed easily later.
> 
> This is no excuse to redo all these bugs. :-)

I agree with you, but what can we do now is trying our best to fuzz
all the fields.

So, what is your opinion about EROFS?

> 
> I know that many in-kernel filesystems trust the disk ultimately, this is a
> problem and huge attack vector.

I EROFS already has many error handing paths to recover corrupted images,
and your discovery is a bug out of one error handing path miswritten by me.
I cannot make a guarantee that all the new things (every new kernel version
will introduce new feature / new code) are bug-free since I am not a machine
or code parser.

My answer about this EROFS will be more stable with our development, we have
a dedicated team with paid job, and since we currently use EROFS on the top of
dm-verity for current Android, which will keep us from corrupted images.
But yes, we will focus on fuzzing all the images for generic usages,
and we will backport all these patches to old stable versions.

Thanks,
Gao Xiang

> 
> Thanks,
> //richard
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation

2019-08-18 Thread Chuanhong Guo
On Sun, Aug 18, 2019 at 4:26 PM Chuanhong Guo  wrote:
>
> Hi!
>
> On Sun, Aug 18, 2019 at 3:59 PM Oleksij Rempel  wrote:
> >
> > Am 18.08.19 um 09:19 schrieb Chuanhong Guo:
> > > Hi!
> > >
> > > On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel  
> > > wrote:
> > >>
> >  We have at least 2 know registers:
> >  SYSC_REG_CPLL_CLKCFG0 - it provides some information about boostrapped
> >  refclock. PLL and dividers used for CPU and some sort of BUS (AHB?).
> >  SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for
> >  all or some ip cores.
> >  What is probably missing is a set of dividers for
> >  each ip core. From your words it is not document.
> > >>>
> > >>> The specific missing part I was referring to, is parent clocks for
> > >>> every gates. I'm not going to assume this with current openwrt device
> > >>> tree because some peripherals doesn't have a clock binding at all or
> > >>> have a dummy one there.
> > >>
> > >> Ok, then I do not understand what is the motivation to upstream
> > >> something what is not nearly ready for use.
> > >
> > > Why isn't it "ready for use" then?
> > > A complete mt7621-pll driver will contain two parts:
> > > 1. A clock provider which outputs several clocks
> > > 2. A clock gate with parent clocks properly configured
> > >
> > > Two clocks provided here are just two clocks that can't be controlled
> > > in kernel no matter where it goes (arch/mips/ralink or drivers/clk).
> > > Having a working CPU clock provider is better than defining a fixed
> > > clock in dts because CPU clock can be controlled by bootloader.
> > > (BTW description for CPU PLL register is also missing in datasheet.)
> > > Clock gate is an unrelated part and there is no information to
> > > properly implement it unless MTK decided to release a clock plan
> > > somehow.
> >
> > With other words, your complete system is running with unknown clock
> > rates.
>
> And without this patchset the complete system is running with unknown
> clock and, even worse, we make assumptions about what clock bootloader
> uses, hardcoded it in dts and hope it is the correct value.
>
> > The source clock in the clock three can be configured differently
> > by bootloader but you don't know how it is done how and it is not
> > documented.
>
> Actually, I don't know about this and I didn't wrote the original
> clock calculation code. I just ported it from downstream OpenWrt
> kernel. Here's a piece of code from Mediatek's SDK kernel:
>
> case 0:
> reg = (*(volatile u32 *)(RALINK_SYSCTL_BASE + 0x44));
> cpu_fdiv = ((reg >> 8) & 0x1F);
> cpu_ffrac = (reg & 0x1F);
> mips_cpu_feq = (500 * cpu_ffrac / cpu_fdiv) * 1000 * 1000;
> break;
> case 1: //CPU PLL
> reg = (*(volatile u32 *)(RALINK_MEMCTRL_BASE + 0x648));
> fbdiv = ((reg >> 4) & 0x7F) + 1;
> reg = (*(volatile u32 *)(RALINK_SYSCTL_BASE + 0x10));
> reg = (reg >> 6) & 0x7;
> if(reg >= 6) { //25Mhz Xtal
> mips_cpu_feq = 25 * fbdiv * 1000 * 1000;
> } else if(reg >=3) { //40Mhz Xtal
> mips_cpu_feq = 20 * fbdiv * 1000 * 1000;
> } else { // 20Mhz Xtal
> /* TODO */
> }
> break;
>
>
>
> >
> > >> This code is currently on prototyping phase
> > >
> > > Code for clock calculation is done, not "prototyping".
> > >
> > >> It means, we cannot expect that this driver will be fixed any time soon.
> > >
> > > I think clock gating is a separated feature instead of a broken part
> > > that has to be fixed.
> >
> > Ok, i would agree with it. But from what you said, this feature will be
> > never implemented.
> >
> > So, I repeat my question. What is the point to upstream code for a
> > system, which has not enough information to get proper clock rate even
> > for uart? or is uart running with cpu or bus clock rate?
>
> uart runs of a fixed 50MHz clock according to another piece of code
> from MTK SDK:
> (a pastebin version here for better readability. This specific
> question has nothing to do with patch reviewing and doesn't need to be
> preserved in mail forever.)
> https://paste.ubuntu.com/p/fYmtDFW9nh/
>
> I could ask the same question:
> What is the point of upstreaming an incomplete MT7621 support in the
> first place? Current MT7621 support in upstream kernel works only for
> mt7621a not mt7621s and it runs of unknown clocks. These kind of code
> should stay in downstream projects like OpenWrt forever isn't it?

And in fact you've upstreamed a broken ag71xx driver anyway.

This debate goes nowhere. I've clarified the situation and made my
point. Of course I can't read through the ancient and heavily hacked
vendor kernel to figure out a clock plan myself.

Regards,
Chuanhong Guo
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()

2019-08-18 Thread Richard Weinberger
- Ursprüngliche Mail -
> changelog from v2:
> - transform EIO to EFSCORRUPTED as suggested by Matthew;

erofs does not define EFSCORRUPTED, so the build fails.
At least on Linus' tree as of today.

Thanks,
//richard
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation

2019-08-18 Thread Chuanhong Guo
Hi!

On Sun, Aug 18, 2019 at 3:59 PM Oleksij Rempel  wrote:
>
> Am 18.08.19 um 09:19 schrieb Chuanhong Guo:
> > Hi!
> >
> > On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel  
> > wrote:
> >>
>  We have at least 2 know registers:
>  SYSC_REG_CPLL_CLKCFG0 - it provides some information about boostrapped
>  refclock. PLL and dividers used for CPU and some sort of BUS (AHB?).
>  SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for
>  all or some ip cores.
>  What is probably missing is a set of dividers for
>  each ip core. From your words it is not document.
> >>>
> >>> The specific missing part I was referring to, is parent clocks for
> >>> every gates. I'm not going to assume this with current openwrt device
> >>> tree because some peripherals doesn't have a clock binding at all or
> >>> have a dummy one there.
> >>
> >> Ok, then I do not understand what is the motivation to upstream
> >> something what is not nearly ready for use.
> >
> > Why isn't it "ready for use" then?
> > A complete mt7621-pll driver will contain two parts:
> > 1. A clock provider which outputs several clocks
> > 2. A clock gate with parent clocks properly configured
> >
> > Two clocks provided here are just two clocks that can't be controlled
> > in kernel no matter where it goes (arch/mips/ralink or drivers/clk).
> > Having a working CPU clock provider is better than defining a fixed
> > clock in dts because CPU clock can be controlled by bootloader.
> > (BTW description for CPU PLL register is also missing in datasheet.)
> > Clock gate is an unrelated part and there is no information to
> > properly implement it unless MTK decided to release a clock plan
> > somehow.
>
> With other words, your complete system is running with unknown clock
> rates.

And without this patchset the complete system is running with unknown
clock and, even worse, we make assumptions about what clock bootloader
uses, hardcoded it in dts and hope it is the correct value.

> The source clock in the clock three can be configured differently
> by bootloader but you don't know how it is done how and it is not
> documented.

Actually, I don't know about this and I didn't wrote the original
clock calculation code. I just ported it from downstream OpenWrt
kernel. Here's a piece of code from Mediatek's SDK kernel:

case 0:
reg = (*(volatile u32 *)(RALINK_SYSCTL_BASE + 0x44));
cpu_fdiv = ((reg >> 8) & 0x1F);
cpu_ffrac = (reg & 0x1F);
mips_cpu_feq = (500 * cpu_ffrac / cpu_fdiv) * 1000 * 1000;
break;
case 1: //CPU PLL
reg = (*(volatile u32 *)(RALINK_MEMCTRL_BASE + 0x648));
fbdiv = ((reg >> 4) & 0x7F) + 1;
reg = (*(volatile u32 *)(RALINK_SYSCTL_BASE + 0x10));
reg = (reg >> 6) & 0x7;
if(reg >= 6) { //25Mhz Xtal
mips_cpu_feq = 25 * fbdiv * 1000 * 1000;
} else if(reg >=3) { //40Mhz Xtal
mips_cpu_feq = 20 * fbdiv * 1000 * 1000;
} else { // 20Mhz Xtal
/* TODO */
}
break;



>
> >> This code is currently on prototyping phase
> >
> > Code for clock calculation is done, not "prototyping".
> >
> >> It means, we cannot expect that this driver will be fixed any time soon.
> >
> > I think clock gating is a separated feature instead of a broken part
> > that has to be fixed.
>
> Ok, i would agree with it. But from what you said, this feature will be
> never implemented.
>
> So, I repeat my question. What is the point to upstream code for a
> system, which has not enough information to get proper clock rate even
> for uart? or is uart running with cpu or bus clock rate?

uart runs of a fixed 50MHz clock according to another piece of code
from MTK SDK:
(a pastebin version here for better readability. This specific
question has nothing to do with patch reviewing and doesn't need to be
preserved in mail forever.)
https://paste.ubuntu.com/p/fYmtDFW9nh/

I could ask the same question:
What is the point of upstreaming an incomplete MT7621 support in the
first place? Current MT7621 support in upstream kernel works only for
mt7621a not mt7621s and it runs of unknown clocks. These kind of code
should stay in downstream projects like OpenWrt forever isn't it?

Regards,
Chuanhong Guo
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] erofs: move erofs out of staging

2019-08-18 Thread Richard Weinberger
- Ursprüngliche Mail -
>> While digging a little into the code I noticed that you have very few
>> checks of the on-disk data.
>> For example ->u.i_blkaddr. I gave it a try and created a
>> malformed filesystem where u.i_blkaddr is 0xdeadbeef, it causes the kernel
>> to loop forever around erofs_read_raw_page().
> 
> I don't fuzz all the on-disk fields for EROFS, I will do later..
> You can see many in-kernel filesystems are still hardening the related
> stuff. Anyway, I will dig into this field you mentioned recently, but
> I think it can be fixed easily later.

This is no excuse to redo all these bugs. :-)

I know that many in-kernel filesystems trust the disk ultimately, this is a
problem and huge attack vector.

Thanks,
//richard
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation

2019-08-18 Thread Oleksij Rempel
Am 18.08.19 um 09:19 schrieb Chuanhong Guo:
> Hi!
>
> On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel  wrote:
>>
 We have at least 2 know registers:
 SYSC_REG_CPLL_CLKCFG0 - it provides some information about boostrapped
 refclock. PLL and dividers used for CPU and some sort of BUS (AHB?).
 SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for
 all or some ip cores.
 What is probably missing is a set of dividers for
 each ip core. From your words it is not document.
>>>
>>> The specific missing part I was referring to, is parent clocks for
>>> every gates. I'm not going to assume this with current openwrt device
>>> tree because some peripherals doesn't have a clock binding at all or
>>> have a dummy one there.
>>
>> Ok, then I do not understand what is the motivation to upstream
>> something what is not nearly ready for use.
>
> Why isn't it "ready for use" then?
> A complete mt7621-pll driver will contain two parts:
> 1. A clock provider which outputs several clocks
> 2. A clock gate with parent clocks properly configured
>
> Two clocks provided here are just two clocks that can't be controlled
> in kernel no matter where it goes (arch/mips/ralink or drivers/clk).
> Having a working CPU clock provider is better than defining a fixed
> clock in dts because CPU clock can be controlled by bootloader.
> (BTW description for CPU PLL register is also missing in datasheet.)
> Clock gate is an unrelated part and there is no information to
> properly implement it unless MTK decided to release a clock plan
> somehow.

With other words, your complete system is running with unknown clock
rates. The source clock in the clock three can be configured differently
by bootloader but you don't know how it is done how and it is not
documented.

>> This code is currently on prototyping phase
>
> Code for clock calculation is done, not "prototyping".
>
>> It means, we cannot expect that this driver will be fixed any time soon.
>
> I think clock gating is a separated feature instead of a broken part
> that has to be fixed.

Ok, i would agree with it. But from what you said, this feature will be
never implemented.

So, I repeat my question. What is the point to upstream code for a
system, which has not enough information to get proper clock rate even
for uart? or is uart running with cpu or bus clock rate?

--
Regards,
Oleksij
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation

2019-08-18 Thread Chuanhong Guo
Hi!

On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel  wrote:
>
> >> We have at least 2 know registers:
> >> SYSC_REG_CPLL_CLKCFG0 - it provides some information about boostrapped
> >> refclock. PLL and dividers used for CPU and some sort of BUS (AHB?).
> >> SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for
> >> all or some ip cores.
> >> What is probably missing is a set of dividers for
> >> each ip core. From your words it is not document.
> >
> > The specific missing part I was referring to, is parent clocks for
> > every gates. I'm not going to assume this with current openwrt device
> > tree because some peripherals doesn't have a clock binding at all or
> > have a dummy one there.
>
> Ok, then I do not understand what is the motivation to upstream
> something what is not nearly ready for use.

Why isn't it "ready for use" then?
A complete mt7621-pll driver will contain two parts:
1. A clock provider which outputs several clocks
2. A clock gate with parent clocks properly configured

Two clocks provided here are just two clocks that can't be controlled
in kernel no matter where it goes (arch/mips/ralink or drivers/clk).
Having a working CPU clock provider is better than defining a fixed
clock in dts because CPU clock can be controlled by bootloader.
(BTW description for CPU PLL register is also missing in datasheet.)
Clock gate is an unrelated part and there is no information to
properly implement it unless MTK decided to release a clock plan
somehow.

> This code is currently on prototyping phase

Code for clock calculation is done, not "prototyping".

> It means, we cannot expect that this driver will be fixed any time soon.

I think clock gating is a separated feature instead of a broken part
that has to be fixed.

Regards,
Chuanhong Guo
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: android: Remove ion device tree bindings from the TODO

2019-08-18 Thread Greg KH
On Sun, Aug 18, 2019 at 01:47:38AM -0400, Donald Yandt wrote:
> On Sun, Aug 18, 2019 at 1:03 AM Greg KH  wrote:
> >
> > On Sat, Aug 17, 2019 at 05:37:58PM -0400, Donald Yandt wrote:
> > > This patch removes the todo for the ion chunk and
> > > carveout device tree bindings.
> > >
> > > Signed-off-by: Donald Yandt 
> > > ---
> > >  drivers/staging/android/TODO | 2 --
> > >  1 file changed, 2 deletions(-)
> > >
> > > diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
> > > index fbf015cc6..767dd98fd 100644
> > > --- a/drivers/staging/android/TODO
> > > +++ b/drivers/staging/android/TODO
> > > @@ -6,8 +6,6 @@ TODO:
> > >
> > >
> > >  ion/
> > > - - Add dt-bindings for remaining heaps (chunk and carveout heaps). This 
> > > would
> > > -   involve putting appropriate bindings in a memory node for Ion to find.
> > >   - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
> > >   - Better test framework (integration with VGEM was suggested)
> > >
> >
> > This is already done?  Do you have a pointer to the git commit id(s)
> > that did it?
> >
> > thanks,
> >
> > greg k-h
> 
> Hi Greg,
> 
> Both the chunk and carveout heaps were removed from ion,
> so unless I'm mistaken there's no need to implement the device tree
> bindings for them.
> 
> Commits that removed both heaps:
>   - 23a4388f2 staging: android: ion: Remove file ion_chunk_heap.c
>   - eadbf7a34 staging: android: ion: Remove file ion_carveout_heap.c

Great, can you resend this patch with that information in the changelog?
As you wrote it, it could be implied that these tasks were actually
completed :)

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation

2019-08-18 Thread Oleksij Rempel
Am 18.08.19 um 04:29 schrieb Chuanhong Guo:
> Hi!
>
> On Sun, Aug 18, 2019 at 2:06 AM Oleksij Rempel  wrote:
 SYSC_REG_CPLL_CLKCFG1 register is a clock gate controller. It is used to 
 enable or disable clocks.
 Jist wild assumption. All peripheral devices are suing bus clock.
>>>
>>> This assumption is incorrect. When this patchset is applied in
>>> OpenWrt, I asked the author why there's still a fixed clock in
>>> mt7621.dtsi, He told me that there's another clock for those unchanged
>>> peripherals and he doesn't have time to write a clock provider for it.
>>
>> Can you please provide a link to this patch or email.
>
> This discussion is in Chinese and using an IM software so there's no
> link available.
>
>> We have at least 2 know registers:
>> SYSC_REG_CPLL_CLKCFG0 - it provides some information about boostrapped
>> refclock. PLL and dividers used for CPU and some sort of BUS (AHB?).
>> SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for
>> all or some ip cores.
>> What is probably missing is a set of dividers for
>> each ip core. From your words it is not document.
>
> The specific missing part I was referring to, is parent clocks for
> every gates. I'm not going to assume this with current openwrt device
> tree because some peripherals doesn't have a clock binding at all or
> have a dummy one there.

Ok, then I do not understand what is the motivation to upstream
something what is not nearly ready for use. This code is currently on
prototyping phase and you have no information how to make it ready.

It means, we cannot expect that this driver will be fixed any time soon.

--
Regards,
Oleksij
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel