Re: [Outreachy kernel] Re: [PATCH v2 1/2] staging: kpc2000: kpc_dma: rearrange lines exceeding 100 columns
On Mon, Oct 26, 2020 at 09:34:53AM +0530, Deepak R Varma wrote: > > - dev_dbg(>pldev->dev, "Handling completed descriptor %p > > (acd = %p)\n", cur, cur->acd); > > + dev_dbg(>pldev->dev, "Handling completed descriptor %p > > (acd = %p)\n", > > + cur, > > + cur->acd); Why do you put 'cur' and 'cur->acd' on different lines? > > - rv = request_irq(eng->irq, ndd_irq_handler, IRQF_SHARED, > > KP_DRIVER_NAME_DMA_CONTROLLER, eng); > > + rv = request_irq(eng->irq, > > +ndd_irq_handler, > > +IRQF_SHARED, > > +KP_DRIVER_NAME_DMA_CONTROLLER, > > +eng); Likewise. I'd do: rv = request_irq(eng->irq, ndd_irq_handler, IRQF_SHARED, KP_DRIVER_NAME_DMA_CONTROLLER, eng); ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH] staging/rtl8192e: replace kmalloc with kzalloc
On Fri, Oct 23, 2020 at 03:03:17AM -0700, Elena Afanasova wrote: > - txb = kmalloc(sizeof(struct rtllib_txb) + (sizeof(u8 *) * nr_frags), > - gfp_mask); > + txb = kzalloc(sizeof(*txb) + (sizeof(u8 *) * nr_frags), gfp_mask); This would be a good opportunity to use struct_size(). ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH 3/3] staging/rtl8712: use BIT macro
On Tue, Oct 20, 2020 at 11:24:39AM -0700, Elena Afanasova wrote: > Reported by checkpatch.pl Checkpatch is wrong. > +++ b/drivers/staging/rtl8712/rtl871x_recv.h > @@ -8,7 +8,7 @@ > #define NR_RECVFRAME 256 > > #define RXFRAME_ALIGN8 > -#define RXFRAME_ALIGN_SZ (1 << RXFRAME_ALIGN) > +#define RXFRAME_ALIGN_SZ BIT(RXFRAME_ALIGN) > > #define MAX_SUBFRAME_COUNT 64 > > -- > 2.25.1 > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/outreachy-kernel/20201020182439.43314-3-eafanasova%40gmail.com. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Ocfs2-devel] [RFC] treewide: cleanup unreachable breaks
On Sun, Oct 18, 2020 at 12:13:35PM -0700, James Bottomley wrote: > On Sun, 2020-10-18 at 19:59 +0100, Matthew Wilcox wrote: > > On Sat, Oct 17, 2020 at 09:09:28AM -0700, t...@redhat.com wrote: > > > clang has a number of useful, new warnings see > > > https://urldefense.com/v3/__https://clang.llvm.org/docs/DiagnosticsReference.html__;!!GqivPVa7Brio!Krxz78O3RKcB9JBMVo_F98FupVhj_jxX60ddN6tKGEbv_cnooXc1nnBmchm-e_O9ieGnyQ$ > > > > > > > Please get your IT department to remove that stupidity. If you > > can't, please send email from a non-Red Hat email address. > > Actually, the problem is at Oracle's end somewhere in the ocfs2 list > ... if you could fix it, that would be great. The usual real mailing > lists didn't get this transformation > > https://lore.kernel.org/bpf/20201017160928.12698-1-t...@redhat.com/ > > but the ocfs2 list archive did: > > https://oss.oracle.com/pipermail/ocfs2-devel/2020-October/015330.html > > I bet Oracle IT has put some spam filter on the list that mangles URLs > this way. *sigh*. I'm sure there's a way. I've raised it with someone who should be able to fix it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Ocfs2-devel] [RFC] treewide: cleanup unreachable breaks
On Sat, Oct 17, 2020 at 09:09:28AM -0700, t...@redhat.com wrote: > clang has a number of useful, new warnings see > https://urldefense.com/v3/__https://clang.llvm.org/docs/DiagnosticsReference.html__;!!GqivPVa7Brio!Krxz78O3RKcB9JBMVo_F98FupVhj_jxX60ddN6tKGEbv_cnooXc1nnBmchm-e_O9ieGnyQ$ > Please get your IT department to remove that stupidity. If you can't, please send email from a non-Red Hat email address. I don't understand why this is a useful warning to fix. What actual problem is caused by the code below? > return and break > > switch (c->x86_vendor) { > case X86_VENDOR_INTEL: > intel_p5_mcheck_init(c); > return 1; > - break; Sure, it's unnecessary, but it's not masking a bug. It's not unclear. Why do we want to enable this warning? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC PKS/PMEM 33/58] fs/cramfs: Utilize new kmap_thread()
On Tue, Oct 13, 2020 at 11:44:29AM -0700, Dan Williams wrote: > On Fri, Oct 9, 2020 at 12:52 PM wrote: > > > > From: Ira Weiny > > > > The kmap() calls in this FS are localized to a single thread. To avoid > > the over head of global PKRS updates use the new kmap_thread() call. > > > > Cc: Nicolas Pitre > > Signed-off-by: Ira Weiny > > --- > > fs/cramfs/inode.c | 10 +- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c > > index 912308600d39..003c014a42ed 100644 > > --- a/fs/cramfs/inode.c > > +++ b/fs/cramfs/inode.c > > @@ -247,8 +247,8 @@ static void *cramfs_blkdev_read(struct super_block *sb, > > unsigned int offset, > > struct page *page = pages[i]; > > > > if (page) { > > - memcpy(data, kmap(page), PAGE_SIZE); > > - kunmap(page); > > + memcpy(data, kmap_thread(page), PAGE_SIZE); > > + kunmap_thread(page); > > Why does this need a sleepable kmap? This looks like a textbook > kmap_atomic() use case. There's a lot of code of this form. Could we perhaps have: static inline void copy_to_highpage(struct page *to, void *vfrom, unsigned int size) { char *vto = kmap_atomic(to); memcpy(vto, vfrom, size); kunmap_atomic(vto); } in linux/highmem.h ? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC PKS/PMEM 22/58] fs/f2fs: Utilize new kmap_thread()
On Mon, Oct 12, 2020 at 12:53:54PM -0700, Ira Weiny wrote: > On Mon, Oct 12, 2020 at 05:44:38PM +0100, Matthew Wilcox wrote: > > On Mon, Oct 12, 2020 at 09:28:29AM -0700, Dave Hansen wrote: > > > kmap_atomic() is always preferred over kmap()/kmap_thread(). > > > kmap_atomic() is _much_ more lightweight since its TLB invalidation is > > > always CPU-local and never broadcast. > > > > > > So, basically, unless you *must* sleep while the mapping is in place, > > > kmap_atomic() is preferred. > > > > But kmap_atomic() disables preemption, so the _ideal_ interface would map > > it only locally, then on preemption make it global. I don't even know > > if that _can_ be done. But this email makes it seem like kmap_atomic() > > has no downsides. > > And that is IIUC what Thomas was trying to solve. > > Also, Linus brought up that kmap_atomic() has quirks in nesting.[1] > > >From what I can see all of these discussions support the need to have > >something > between kmap() and kmap_atomic(). > > However, the reason behind converting call sites to kmap_thread() are > different > between Thomas' patch set and mine. Both require more kmap granularity. > However, they do so with different reasons and underlying implementations but > with the _same_ resulting semantics; a thread local mapping which is > preemptable.[2] Therefore they each focus on changing different call sites. > > While this patch set is huge I think it serves a valuable purpose to identify > a > large number of call sites which are candidates for this new semantic. Yes, I agree. My problem with this patch-set is that it ties it to some Intel feature that almost nobody cares about. Maybe we should care about it, but you didn't try very hard to make anyone care about it in the cover letter. For a future patch-set, I'd like to see you just introduce the new API. Then you can optimise the Intel implementation of it afterwards. Those patch-sets have entirely different reviewers. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC PKS/PMEM 22/58] fs/f2fs: Utilize new kmap_thread()
On Mon, Oct 12, 2020 at 09:28:29AM -0700, Dave Hansen wrote: > kmap_atomic() is always preferred over kmap()/kmap_thread(). > kmap_atomic() is _much_ more lightweight since its TLB invalidation is > always CPU-local and never broadcast. > > So, basically, unless you *must* sleep while the mapping is in place, > kmap_atomic() is preferred. But kmap_atomic() disables preemption, so the _ideal_ interface would map it only locally, then on preemption make it global. I don't even know if that _can_ be done. But this email makes it seem like kmap_atomic() has no downsides. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC PKS/PMEM 22/58] fs/f2fs: Utilize new kmap_thread()
On Fri, Oct 09, 2020 at 02:34:34PM -0700, Eric Biggers wrote: > On Fri, Oct 09, 2020 at 12:49:57PM -0700, ira.we...@intel.com wrote: > > The kmap() calls in this FS are localized to a single thread. To avoid > > the over head of global PKRS updates use the new kmap_thread() call. > > > > @@ -2410,12 +2410,12 @@ static inline struct page *f2fs_pagecache_get_page( > > > > static inline void f2fs_copy_page(struct page *src, struct page *dst) > > { > > - char *src_kaddr = kmap(src); > > - char *dst_kaddr = kmap(dst); > > + char *src_kaddr = kmap_thread(src); > > + char *dst_kaddr = kmap_thread(dst); > > > > memcpy(dst_kaddr, src_kaddr, PAGE_SIZE); > > - kunmap(dst); > > - kunmap(src); > > + kunmap_thread(dst); > > + kunmap_thread(src); > > } > > Wouldn't it make more sense to switch cases like this to kmap_atomic()? > The pages are only mapped to do a memcpy(), then they're immediately unmapped. Maybe you missed the earlier thread from Thomas trying to do something similar for rather different reasons ... https://lore.kernel.org/lkml/20200919091751.06...@linutronix.de/ ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] tasklet: Introduce new initialization API
On Wed, Jul 15, 2020 at 08:08:47PM -0700, Kees Cook wrote: > +#define DECLARE_TASKLET(name, _callback) \ > +struct tasklet_struct name = { \ > + .count = ATOMIC_INIT(0),\ > + .callback = _callback, \ > + .use_callback = true, \ > +} > + > +#define DECLARE_TASKLET_DISABLED(name, _callback)\ > +struct tasklet_struct name = { \ > + .count = ATOMIC_INIT(1),\ > + .callback = _callback, \ > +} You forgot to set use_callback here. > @@ -547,7 +547,10 @@ static void tasklet_action_common(struct softirq_action > *a, > if (!test_and_clear_bit(TASKLET_STATE_SCHED, > >state)) > BUG(); > - t->func(t->data); > + if (t->use_callback) > + t->callback(t); > + else > + t->func(t->data); I think this is the wrong way to do the conversion. Start out by setting t->data to (unsigned long)t in the new initialisers. Then convert the drivers (all 350 of them) to the new API. Then you can get rid of 'data' from the tasklet_struct. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3] treewide: Replace DECLARE_TASKLET() with DECLARE_TASKLET_OLD()
On Wed, Jul 15, 2020 at 08:08:46PM -0700, Kees Cook wrote: > This converts all the existing DECLARE_TASKLET() (and ...DISABLED) > macros with DECLARE_TASKLET_OLD() in preparation for refactoring the > tasklet callback type. All existing DECLARE_TASKLET() users had a "0" > data argument, it has been removed here as well. > > Signed-off-by: Kees Cook [...] > 16 files changed, 26 insertions(+), 21 deletions(-) This is about 5% of what needs to change. There are 350 callers of tasklet_init(), and that still takes a 'data' argument. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 0/3] mm, treewide: Rename kzfree() to kfree_sensitive()
On Wed, Jun 17, 2020 at 01:31:57PM +0200, Michal Hocko wrote: > On Wed 17-06-20 04:08:20, Matthew Wilcox wrote: > > If you call vfree() under > > a spinlock, you're in trouble. in_atomic() only knows if we hold a > > spinlock for CONFIG_PREEMPT, so it's not safe to check for in_atomic() > > in __vfree(). So we need the warning in order that preempt people can > > tell those without that there is a bug here. > > ... Unless I am missing something in_interrupt depends on preempt_count() as > well so neither of the two is reliable without PREEMPT_COUNT configured. preempt_count() always tracks whether we're in interrupt context, regardless of CONFIG_PREEMPT. The difference is that CONFIG_PREEMPT will track spinlock acquisitions as well. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 0/3] mm, treewide: Rename kzfree() to kfree_sensitive()
On Wed, Jun 17, 2020 at 09:12:12AM +0200, Michal Hocko wrote: > On Tue 16-06-20 17:37:11, Matthew Wilcox wrote: > > Not just performance critical, but correctness critical. Since kvfree() > > may allocate from the vmalloc allocator, I really think that kvfree() > > should assert that it's !in_atomic(). Otherwise we can get into trouble > > if we end up calling vfree() and have to take the mutex. > > FWIW __vfree already checks for atomic context and put the work into a > deferred context. So this should be safe. It should be used as a last > resort, though. Actually, it only checks for in_interrupt(). If you call vfree() under a spinlock, you're in trouble. in_atomic() only knows if we hold a spinlock for CONFIG_PREEMPT, so it's not safe to check for in_atomic() in __vfree(). So we need the warning in order that preempt people can tell those without that there is a bug here. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 0/3] mm, treewide: Rename kzfree() to kfree_sensitive()
On Wed, Jun 17, 2020 at 01:01:30AM +0200, David Sterba wrote: > On Tue, Jun 16, 2020 at 11:53:50AM -0700, Joe Perches wrote: > > On Mon, 2020-06-15 at 21:57 -0400, Waiman Long wrote: > > > v4: > > > - Break out the memzero_explicit() change as suggested by Dan Carpenter > > > so that it can be backported to stable. > > > - Drop the "crypto: Remove unnecessary memzero_explicit()" patch for > > > now as there can be a bit more discussion on what is best. It will be > > > introduced as a separate patch later on after this one is merged. > > > > To this larger audience and last week without reply: > > https://lore.kernel.org/lkml/573b3fbd5927c643920e1364230c296b23e7584d.ca...@perches.com/ > > > > Are there _any_ fastpath uses of kfree or vfree? > > I'd consider kfree performance critical for cases where it is called > under locks. If possible the kfree is moved outside of the critical > section, but we have rbtrees or lists that get deleted under locks and > restructuring the code to do eg. splice and free it outside of the lock > is not always possible. Not just performance critical, but correctness critical. Since kvfree() may allocate from the vmalloc allocator, I really think that kvfree() should assert that it's !in_atomic(). Otherwise we can get into trouble if we end up calling vfree() and have to take the mutex. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 0/3] mm, treewide: Rename kzfree() to kfree_sensitive()
On Tue, Jun 16, 2020 at 11:53:50AM -0700, Joe Perches wrote: > To this larger audience and last week without reply: > https://lore.kernel.org/lkml/573b3fbd5927c643920e1364230c296b23e7584d.ca...@perches.com/ > > Are there _any_ fastpath uses of kfree or vfree? I worked on adding a 'free' a couple of years ago. That was capable of freeing percpu, vmalloc, kmalloc and alloc_pages memory. I ran into trouble when I tried to free kmem_cache_alloc memory -- it works for slab and slub, but not slob (because slob needs the size from the kmem_cache). My motivation for this was to change kfree_rcu() to just free_rcu(). > To eliminate these mispairings at a runtime cost of four > comparisons, should the kfree/vfree/kvfree/kfree_const > functions be consolidated into a single kfree? I would say to leave kfree() alone and just introduce free() as a new default. There's some weird places in the kernel that have a 'free' symbol of their own, but those should be renamed anyway. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: exfat: Fix checkpatch.pl camelcase issues
On Thu, Mar 19, 2020 at 10:06:47AM -0400, Aravind Ceyardass wrote: > Fix ffsCamelCase function names and mixed case enums This driver is now gone from staging in -next; please review the code in fs/exfat instead. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: octeon: delete driver
On Wed, Feb 05, 2020 at 06:34:16AM +0300, Dan Carpenter wrote: > On Tue, Feb 04, 2020 at 12:31:16PM -0800, Matthew Wilcox wrote: > > On Tue, Feb 04, 2020 at 08:06:14PM +, Chris Packham wrote: > > > On Tue, 2020-02-04 at 07:09 +, gre...@linuxfoundation.org wrote: > > > > On Tue, Feb 04, 2020 at 04:02:15AM +, Chris Packham wrote: > > > On Tue, 2020-02-04 at 10:21 +0300, Dan Carpenter wrote: > > > > My advice is to delete all the COMPILE_TEST code. That stuff was a > > > > constant source of confusion and headaches. > > > > > > I was also going to suggest this. Since the COMPILE_TEST has been a > > > source of trouble I was going to propose dropping the || COMPILE_TEST > > > from the Kconfig for the octeon drivers. > > > > Not having it also causes problems. I didn't originally add it for > > shits and giggles. > > I wonder if the kbuild bot does enough cross compile build testing these > days to detect compile problems. It might have improved to the point > where COMPILE_TEST isn't required. Well, that was the problem. I posted the patch and Dave Miller merged it before the build bot had the chance to point out that I'd missed it. So relying on the build bot is not sufficient. > One of the things about having a bunch of dummy functions for > COMPILE_TEST is that they introduce a lot of static checker warnings. > The real function is supposed to initialize stuff but the dummy function > just returns so now we get uninitialized variable warnings etc. Perhaps we need a better solution for the dummy functions than just returning. We can initialise the variables / structs to 0, for example. I fully accept that I did a poor job of writing the dummy functions. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: octeon: delete driver
On Tue, Feb 04, 2020 at 08:06:14PM +, Chris Packham wrote: > On Tue, 2020-02-04 at 07:09 +, gre...@linuxfoundation.org wrote: > > On Tue, Feb 04, 2020 at 04:02:15AM +, Chris Packham wrote: > > > I'll pipe up on this thread too > > > > > > On Tue, 2019-12-10 at 02:42 -0800, Guenter Roeck wrote: > > > > On 12/10/19 1:15 AM, Greg Kroah-Hartman wrote: > > > > > This driver has been in the tree since 2009 with no real movement to > > > > > get > > > > > it out. Now it is starting to cause build issues and other problems > > > > > for > > > > > people who want to fix coding style problems, but can not actually > > > > > build > > > > > it. > > > > > > > > > > As nothing is happening here, just delete the module entirely. > > > > > > > > > > Reported-by: Guenter Roeck > > > > > Cc: David Daney > > > > > Cc: "David S. Miller" > > > > > Cc: "Matthew Wilcox (Oracle)" > > > > > Cc: Guenter Roeck > > > > > Cc: YueHaibing > > > > > Cc: Aaro Koskinen > > > > > Cc: Wambui Karuga > > > > > Cc: Julia Lawall > > > > > Cc: Florian Westphal > > > > > Cc: Geert Uytterhoeven > > > > > Cc: Branden Bonaby > > > > > Cc: "Petr Štetiar" > > > > > Cc: Sandro Volery > > > > > Cc: Paul Burton > > > > > Cc: Dan Carpenter > > > > > Cc: Giovanni Gherdovich > > > > > Cc: Valery Ivanov > > > > > Signed-off-by: Greg Kroah-Hartman > > > > > > > > Acked-by: Guenter Roeck > > > > > > Please can we keep this driver. We do have platforms using it and we > > > would like it to stay around. > > > > > > Clearly we'll need to sort things out to a point where they build > > > successfully. We've been hoping to see this move out of staging ever > > > since we selected Cavium as a vendor. > > > > Great, can you send me a patchset that reverts this and fixes the build > > issues and accept maintainership of the code? > > > > Yep will do. > > On Tue, 2020-02-04 at 10:21 +0300, Dan Carpenter wrote: > > My advice is to delete all the COMPILE_TEST code. That stuff was a > > constant source of confusion and headaches. > > I was also going to suggest this. Since the COMPILE_TEST has been a > source of trouble I was going to propose dropping the || COMPILE_TEST > from the Kconfig for the octeon drivers. Not having it also causes problems. I didn't originally add it for shits and giggles. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: exfat: remove DOSNAMEs.
On Mon, Feb 03, 2020 at 08:15:59AM +, Greg Kroah-Hartman wrote: > On Mon, Feb 03, 2020 at 12:05:32AM -0800, Matthew Wilcox wrote: > > On Tue, Feb 04, 2020 at 01:31:17AM +0900, Tetsuhiro Kohada wrote: > > > remove 'dos_name','ShortName' and related definitions. > > > > > > 'dos_name' and 'ShortName' are definitions before VFAT. > > > These are never used in exFAT. > > > > Why are we still seeing patches for the exfat in staging? > > Because people like doing cleanup patches :) Sure, but I think people also like to believe that their cleanup patches are making a difference. In this case, they're just churning code that's only weeks away from deletion. > > Why are people not working on the Samsung code base? > > They are, see the patches on the list, hopefully they get merged after > -rc1 is out. I meant the cleanup people. Obviously _some_ people are working on the Samsung codebase. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: exfat: remove DOSNAMEs.
On Tue, Feb 04, 2020 at 01:31:17AM +0900, Tetsuhiro Kohada wrote: > remove 'dos_name','ShortName' and related definitions. > > 'dos_name' and 'ShortName' are definitions before VFAT. > These are never used in exFAT. Why are we still seeing patches for the exfat in staging? Why are people not working on the Samsung code base? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: octeon: fix missing a blank line after declaration
I would like to reiterate my opinion that this checkpatch warning is bullshit. For large functions, sure. For this kind of function, it's a waste of space. On Fri, Nov 08, 2019 at 02:23:29PM +, Valery Ivanov wrote: > This patch fixes "WARNING: Missing a blank line after declarations" > Issue found by checkpatch.pl > > Signed-off-by: Valery Ivanov > --- > Changes in v2: > - fix huge indentation in commit message > --- > drivers/staging/octeon/octeon-stubs.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/staging/octeon/octeon-stubs.h > b/drivers/staging/octeon/octeon-stubs.h > index d53bd801f440..ed9d44ff148b 100644 > --- a/drivers/staging/octeon/octeon-stubs.h > +++ b/drivers/staging/octeon/octeon-stubs.h > @@ -1375,6 +1375,7 @@ static inline union cvmx_gmxx_rxx_rx_inbnd > cvmx_spi4000_check_speed( > int port) > { > union cvmx_gmxx_rxx_rx_inbnd r; > + > r.u64 = 0; > return r; > } > -- > 2.17.1 > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/15] staging: exfat: Clean up return codes - FFS_FULL
On Thu, Oct 24, 2019 at 11:53:12AM -0400, Valdis Kletnieks wrote: > Start cleaning up the odd scheme of return codes, starting with FFS_FULL > +++ b/drivers/staging/exfat/exfat.h > @@ -221,7 +221,6 @@ static inline u16 get_row_index(u16 i) > #define FFS_PERMISSIONERR 11 > #define FFS_NOTOPENED 12 > #define FFS_MAXOPENED 13 > -#define FFS_FULL14 > #define FFS_EOF 15 > #define FFS_DIRBUSY 16 > #define FFS_MEMORYERR 17 Wouldn't it be better to do this as: Patch 1: Change all these defines to -Exxx and remove the stupid errno-changing blocks like this: > @@ -2360,7 +2360,7 @@ static int exfat_create(struct inode *dir, struct > dentry *dentry, umode_t mode, > err = -EINVAL; > else if (err == FFS_FILEEXIST) > err = -EEXIST; > - else if (err == FFS_FULL) > + else if (err == -ENOSPC) > err = -ENOSPC; > else if (err == FFS_NAMETOOLONG) > err = -ENAMETOOLONG; then patches 2-n remove individual FFS error codes. That way nobody actually needs to review patches 2-n; all of the changes are done in patch 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()
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 v2] staging: erofs: fix an error handling in erofs_readdir()
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; } 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()
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/... ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] net: mdio-octeon: Fix build error and Kconfig warning
On Fri, Aug 02, 2019 at 06:30:31PM -0700, Nathan Chancellor wrote: > On Fri, Aug 02, 2019 at 06:11:32PM -0700, David Miller wrote: > > The proper way to fix this is to include either > > > > linux/io-64-nonatomic-hi-lo.h > > > > or > > > > linux/io-64-nonatomic-lo-hi.h > > > > whichever is appropriate. > > H, is that not what I did? > > Although I did not know about io-64-nonatomic-hi-lo.h. What is the > difference and which one is needed here? Whether you write the high or low 32 bits first. For this, it doesn't matter, since the compiled driver will never be run on real hardware. > There is apparently another failure when OF_MDIO is not set, I guess I > can try to look into that as well and respin into a series if > necessary. Thanks for taking care of that! ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/34] put_user_pages(): miscellaneous call sites
On Fri, Aug 02, 2019 at 02:41:46PM +0200, Jan Kara wrote: > On Fri 02-08-19 11:12:44, Michal Hocko wrote: > > On Thu 01-08-19 19:19:31, john.hubb...@gmail.com wrote: > > [...] > > > 2) Convert all of the call sites for get_user_pages*(), to > > > invoke put_user_page*(), instead of put_page(). This involves dozens of > > > call sites, and will take some time. > > > > How do we make sure this is the case and it will remain the case in the > > future? There must be some automagic to enforce/check that. It is simply > > not manageable to do it every now and then because then 3) will simply > > be never safe. > > > > Have you considered coccinele or some other scripted way to do the > > transition? I have no idea how to deal with future changes that would > > break the balance though. > > Yeah, that's why I've been suggesting at LSF/MM that we may need to create > a gup wrapper - say vaddr_pin_pages() - and track which sites dropping > references got converted by using this wrapper instead of gup. The > counterpart would then be more logically named as unpin_page() or whatever > instead of put_user_page(). Sure this is not completely foolproof (you can > create new callsite using vaddr_pin_pages() and then just drop refs using > put_page()) but I suppose it would be a high enough barrier for missed > conversions... Thoughts? I think the API we really need is get_user_bvec() / put_user_bvec(), and I know Christoph has been putting some work into that. That avoids doing refcount operations on hundreds of pages if the page in question is a huge page. Once people are switched over to that, they won't be tempted to manually call put_page() on the individual constituent pages of a bvec. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
exfat filesystem
On Tue, Jul 09, 2019 at 11:30:39AM -0400, Theodore Ts'o wrote: > On Tue, Jul 09, 2019 at 04:21:36AM -0700, Matthew Wilcox wrote: > > How does > > https://www.zdnet.com/article/microsoft-open-sources-its-entire-patent-portfolio/ > > change your personal opinion? > > According to SFC's legal analysis, Microsoft joining the OIN doesn't > mean that the eXFAT patents are covered, unless *Microsoft* > contributes the code to the Linux usptream kernel. That's because the > OIN is governed by the Linux System Definition, and until MS > contributes code which covered by the exFAT patents, it doesn't count. > > For more details: > > https://sfconservancy.org/blog/2018/oct/10/microsoft-oin-exfat/ > > (This is not legal advice, and I am not a lawyer.) Interesting analysis. It seems to me that the correct forms would be observed if someone suitably senior at Microsoft accepted the work from Valdis and submitted it with their sign-off. KY, how about it? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Procedure questions - new filesystem driver..
On Tue, Jul 09, 2019 at 12:50:20AM -0400, Theodore Ts'o wrote: > How have you dealt with the patent claims which Microsoft has > asserted[1] on the exFAT file system design? > > [1] > https://www.microsoft.com/en-us/legal/intellectualproperty/mtl/exfat-licensing.aspx > > I am not making any claims about the validity of Microsoft's patent > assertions on exFAT, one way or another. But it might be a good idea > for some laywers from the Linux Foundation to render some legal advice > to their employees (namely Greg K-H and Linus Torvalds) regarding the > advisability of taking exFAT into the official Linux tree. > > Personally, if Microsoft is going to be unfriendly about not wanting > others to use their file system technology by making patent claims, > why should we reward them by making their file system better by > improvings its interoperability? (My personal opinion only.) How does https://www.zdnet.com/article/microsoft-open-sources-its-entire-patent-portfolio/ change your personal opinion? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC] simple_lmk: Introduce Simple Low Memory Killer for Android
On Tue, Mar 12, 2019 at 09:05:32AM +0100, Michal Hocko wrote: > On Mon 11-03-19 15:15:35, Suren Baghdasaryan wrote: > > Yeah, killing speed is a well-known problem which we are considering > > in LMKD. For example the recent LMKD change to assign process being > > killed to a cpuset cgroup containing big cores cuts the kill time > > considerably. This is not ideal and we are thinking about better ways > > to expedite the cleanup process. > > If you design is relies on the speed of killing then it is fundamentally > flawed AFAICT. You cannot assume anything about how quickly a task dies. > It might be blocked in an uninterruptible sleep or performin an > operation which takes some time. Sure, oom_reaper might help here but > still. Many UNINTERRUPTIBLE sleeps can be converted to KILLABLE sleeps. It just needs someone to do the work. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC 2/6] mm: convert PG_balloon to PG_offline
On Wed, Nov 14, 2018 at 10:17:00PM +0100, David Hildenbrand wrote: > Rename PG_balloon to PG_offline. This is an indicator that the page is > logically offline, the content stale and that it should not be touched > (e.g. a hypervisor would have to allocate backing storage in order for the > guest to dump an unused page). We can then e.g. exclude such pages from > dumps. > > In following patches, we will make use of this bit also in other balloon > drivers. While at it, document PGTABLE. Thank you for documenting PGTABLE. I didn't realise I also had this document to update when I added PGTABLE. > +++ b/Documentation/admin-guide/mm/pagemap.rst > @@ -78,6 +78,8 @@ number of times a page is mapped. > 23. BALLOON > 24. ZERO_PAGE > 25. IDLE > +26. PGTABLE > +27. OFFLINE So the offline *user* bit is new ... even though the *kernel* bit just renames the balloon bit. I'm not sure how I feel about this. I'm going to think about it some more. Could you share your decision process with us? I have no objection to renaming the balloon bit inside the kernel; I think that's a wise idea. I'm just not sure whether we should rename the user balloon bit rather than adding a new bit. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v7 1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set array
> +++ b/drivers/auxdisplay/hd44780.c > @@ -62,17 +62,12 @@ static void hd44780_strobe_gpio(struct hd44780 *hd) > /* write to an LCD panel register in 8 bit GPIO mode */ > static void hd44780_write_gpio8(struct hd44780 *hd, u8 val, unsigned int rs) > { > - int values[10]; /* for DATA[0-7], RS, RW */ > - unsigned int i, n; > - > - for (i = 0; i < 8; i++) > - values[PIN_DATA0 + i] = !!(val & BIT(i)); > - values[PIN_CTRL_RS] = rs; > - n = 9; > - if (hd->pins[PIN_CTRL_RW]) { > - values[PIN_CTRL_RW] = 0; > - n++; > - } > + DECLARE_BITMAP(values, 10); /* for DATA[0-7], RS, RW */ > + unsigned int n; > + > + *values = val; > + __assign_bit(8, values, rs); > + n = hd->pins[PIN_CTRL_RW] ? 10 : 9; Doesn't this assume little endian bitmaps? Has anyone tested this on big-endian machines? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: lustre: Change return type to vm_fault_t
On Mon, Apr 23, 2018 at 11:38:17PM +0530, Souptick Joarder wrote: > On Mon, Apr 23, 2018 at 11:05 PM, Matthew Wilcox <wi...@infradead.org> wrote: > > I think you need to check your setup ... I get this: > > > > drivers/staging/lustre/lustre/llite/llite_mmap.c:277:31: warning: incorrect > > type in return expression (different base types) > > drivers/staging/lustre/lustre/llite/llite_mmap.c:277:31:expected > > restricted vm_fault_t > > drivers/staging/lustre/lustre/llite/llite_mmap.c:277:31:got long > > What I am trying is - > after applying the patch in 4.17-rc1, enable the configuration > for this driver and do "make c=1/2 -j8" > > Let me verify again. Yep, but you need to add the sparse annotations to make vm_fault_t a separate type. In case you've lost that patch ... diff --git a/include/linux/mm.h b/include/linux/mm.h index ad06d42adb1a..9ac6d2eb633f 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1174,52 +1175,6 @@ static inline void clear_page_pfmemalloc(struct page *page) page->index = 0; } -/* - * Different kinds of faults, as returned by handle_mm_fault(). - * Used to decide whether a process gets delivered SIGBUS or - * just gets major/minor fault counters bumped up. - */ - -#define VM_FAULT_OOM 0x0001 -#define VM_FAULT_SIGBUS0x0002 -#define VM_FAULT_MAJOR 0x0004 -#define VM_FAULT_WRITE 0x0008 /* Special case for get_user_pages */ -#define VM_FAULT_HWPOISON 0x0010 /* Hit poisoned small page */ -#define VM_FAULT_HWPOISON_LARGE 0x0020 /* Hit poisoned large page. Index encoded in upper bits */ -#define VM_FAULT_SIGSEGV 0x0040 - -#define VM_FAULT_NOPAGE0x0100 /* ->fault installed the pte, not return page */ -#define VM_FAULT_LOCKED0x0200 /* ->fault locked the returned page */ -#define VM_FAULT_RETRY 0x0400 /* ->fault blocked, must retry */ -#define VM_FAULT_FALLBACK 0x0800 /* huge page fault failed, fall back to small */ -#define VM_FAULT_DONE_COW 0x1000 /* ->fault has fully handled COW */ -#define VM_FAULT_NEEDDSYNC 0x2000 /* ->fault did not modify page tables -* and needs fsync() to complete (for -* synchronous page faults in DAX) */ - -#define VM_FAULT_ERROR (VM_FAULT_OOM | VM_FAULT_SIGBUS | VM_FAULT_SIGSEGV | \ -VM_FAULT_HWPOISON | VM_FAULT_HWPOISON_LARGE | \ -VM_FAULT_FALLBACK) - -#define VM_FAULT_RESULT_TRACE \ - { VM_FAULT_OOM, "OOM" }, \ - { VM_FAULT_SIGBUS, "SIGBUS" }, \ - { VM_FAULT_MAJOR, "MAJOR" }, \ - { VM_FAULT_WRITE, "WRITE" }, \ - { VM_FAULT_HWPOISON,"HWPOISON" }, \ - { VM_FAULT_HWPOISON_LARGE, "HWPOISON_LARGE" }, \ - { VM_FAULT_SIGSEGV, "SIGSEGV" }, \ - { VM_FAULT_NOPAGE, "NOPAGE" }, \ - { VM_FAULT_LOCKED, "LOCKED" }, \ - { VM_FAULT_RETRY, "RETRY" }, \ - { VM_FAULT_FALLBACK,"FALLBACK" }, \ - { VM_FAULT_DONE_COW,"DONE_COW" }, \ - { VM_FAULT_NEEDDSYNC, "NEEDDSYNC" } - -/* Encode hstate index for a hwpoisoned large page */ -#define VM_FAULT_SET_HINDEX(x) ((x) << 12) -#define VM_FAULT_GET_HINDEX(x) (((x) >> 12) & 0xf) - /* * Can be called by the pagefault handler when it gets a VM_FAULT_OOM. */ diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h index fd1af6b9591d..5265f425fe88 100644 --- a/include/linux/mm_types.h +++ b/include/linux/mm_types.h @@ -610,6 +610,54 @@ static inline bool mm_tlb_flush_nested(struct mm_struct *mm) struct vm_fault; +/* + * Different kinds of faults, as returned from fault handlers. + * Used to decide whether a process gets delivered SIGBUS or + * just gets major/minor fault counters bumped up. + */ +typedef __bitwise unsigned int vm_fault_t; +enum { + VM_FAULT_OOM= (__force vm_fault_t)0x01, /* Out Of Memory */ + VM_FAULT_SIGBUS = (__force vm_fault_t)0x02, /* Bad access */ + VM_FAULT_MAJOR = (__force vm_fault_t)0x04, /* Page read from storage */ + VM_FAULT_WRITE = (__force vm_fault_t)0x08, /* Special case for get_user_pages */ + VM_FAULT_HWPOISON = (__force vm_fault_t)0x10, /* Hit poisoned small page */ + VM_FAULT_HWPOISON_LARGE = (__force vm_fault_t)0x20, /* Hit poisoned large page. Index encoded in upper bits */ + VM_FAULT_SIGSEGV= (__force vm_fault_t)0x40, + VM_FAULT_NOPAGE = (__force vm_fault_t)0x000100, /* ->fault installed the pte, not return page */ + VM_FAULT_LOCKED = (__for
Re: [PATCH] staging: lustre: Change return type to vm_fault_t
On Mon, Apr 23, 2018 at 10:12:30PM +0530, Souptick Joarder wrote: > On Sun, Apr 22, 2018 at 8:50 AM, Matthew Wilcox <wi...@infradead.org> wrote: > > On Sun, Apr 22, 2018 at 03:47:24AM +0530, Souptick Joarder wrote: > >> @@ -261,7 +261,7 @@ static inline int to_fault_error(int result) > >> * \retval VM_FAULT_ERROR on general error > >> * \retval NOPAGE_OOM not have memory for allocate new page > >> */ > >> -static int ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf) > >> +static vm_fault_t ll_fault0(struct vm_area_struct *vma, struct vm_fault > >> *vmf) > >> { > >> struct lu_env *env; > >> struct cl_io*io; > > > > Did you compile-test this with the sparse changes? Because I can see > > a problem here: > > Yes, compile-tested. Sparse didn't throw any warning/error. I think you need to check your setup ... I get this: drivers/staging/lustre/lustre/llite/llite_mmap.c:277:31: warning: incorrect type in return expression (different base types) drivers/staging/lustre/lustre/llite/llite_mmap.c:277:31:expected restricted vm_fault_t drivers/staging/lustre/lustre/llite/llite_mmap.c:277:31:got long ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: lustre: Change return type to vm_fault_t
On Sun, Apr 22, 2018 at 03:47:24AM +0530, Souptick Joarder wrote: > @@ -261,7 +261,7 @@ static inline int to_fault_error(int result) > * \retval VM_FAULT_ERROR on general error > * \retval NOPAGE_OOM not have memory for allocate new page > */ > -static int ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf) > +static vm_fault_t ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf) > { > struct lu_env *env; > struct cl_io*io; Did you compile-test this with the sparse changes? Because I can see a problem here: env = cl_env_get(); if (IS_ERR(env)) return PTR_ERR(env); > @@ -269,7 +269,7 @@ static int ll_fault0(struct vm_area_struct *vma, struct > vm_fault *vmf) > struct page *vmpage; > unsigned long ra_flags; > int result = 0; > - int fault_ret = 0; > + vm_fault_t fault_ret = 0; What odd indentation ... ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 2/2] ncpfs: move net/ncpfs to drivers/staging/ncpfs
On Sun, Nov 12, 2017 at 11:22:27AM -0800, Stephen Hemminger wrote: > The Netware Core Protocol is a file system that talks to > Netware clients over IPX. Since IPX has been dead for many years > move the file system into staging for eventual interment. Should probably cc Petr on this ... (added) ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel