Re: [PATCH v1 03/10] KVM: Prepare kvm_is_reserved_pfn() for PG_reserved changes
On 06.11.19 01:08, Dan Williams wrote:
On Tue, Nov 5, 2019 at 4:03 PM Sean Christopherson
wrote:
On Tue, Nov 05, 2019 at 03:43:29PM -0800, Dan Williams wrote:
On Tue, Nov 5, 2019 at 3:30 PM Dan Williams wrote:
On Tue, Nov 5, 2019 at 3:13 PM Sean Christopherson
wrote:
On Tue, Nov 05, 2019 at 03:02:40PM -0800, Dan Williams wrote:
On Tue, Nov 5, 2019 at 12:31 PM David Hildenbrand wrote:
The scarier code (for me) is transparent_hugepage_adjust() and
kvm_mmu_zap_collapsible_spte(), as I don't at all understand the
interaction between THP and _PAGE_DEVMAP.
The x86 KVM MMU code is one of the ugliest code I know (sorry, but it
had to be said :/ ). Luckily, this should be independent of the
PG_reserved thingy AFAIKs.
Both transparent_hugepage_adjust() and kvm_mmu_zap_collapsible_spte()
are honoring kvm_is_reserved_pfn(), so again I'm missing where the
page count gets mismanaged and leads to the reported hang.
When mapping pages into the guest, KVM gets the page via gup(), which
increments the page count for ZONE_DEVICE pages. But KVM puts the page
using kvm_release_pfn_clean(), which skips put_page() if PageReserved()
and so never puts its reference to ZONE_DEVICE pages.
Oh, yeah, that's busted.
Ugh, it's extra busted because every other gup user in the kernel
tracks the pages resulting from gup and puts them (put_page()) when
they are done. KVM wants to forget about whether it did a gup to get
the page and optionally trigger put_page() based purely on the pfn.
Outside of VFIO device assignment that needs pages pinned for DMA, why
does KVM itself need to pin pages? If pages are pinned over a return
to userspace that needs to be a FOLL_LONGTERM gup.
Short answer, KVM pins the page to ensure correctness with respect to the
primary MMU invalidating the associated host virtual address, e.g. when
the page is being migrated or unmapped from host userspace.
The main use of gup() is to handle guest page faults and map pages into
the guest, i.e. into KVM's secondary MMU. KVM uses gup() to both get the
PFN and to temporarily pin the page. The pin is held just long enough to
guaranteed that any invalidation via the mmu_notifier will be stalled
until after KVM finishes installing the page into the secondary MMU, i.e.
the pin is short-term and not held across a return to userspace or entry
into the guest. When a subsequent mmu_notifier invalidation occurs, KVM
pulls the PFN from the secondary MMU and uses that to update accessed
and dirty bits in the host.
There are a few other KVM flows that eventually call into gup(), but those
are "traditional" short-term pins and use put_page() directly.
Ok, I was misinterpreting the effect of the bug with what KVM is using
the reference to do.
To your other point:
But David's proposed fix for the above refcount bug is to omit the patch
so that KVM no longer treats ZONE_DEVICE pages as reserved. That seems
like the right thing to do, including for thp_adjust(), e.g. it would
naturally let KVM use 2mb pages for the guest when a ZONE_DEVICE page is
mapped with a huge page (2mb or above) in the host. The only hiccup is
figuring out how to correctly transfer the reference.
That might not be the only hiccup. There's currently no such thing as
huge pages for ZONE_DEVICE, there are huge *mappings* (pmd and pud),
but the result of pfn_to_page() on such a mapping does not yield a
huge 'struct page'. It seems there are other paths in KVM that assume
that more typical page machinery is active like SetPageDirty() based
on kvm_is_reserved_pfn(). While I told David that I did not want to
see more usage of is_zone_device_page(), this patch below (untested)
seems a cleaner path with less surprises:
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 4df0aa6b8e5c..fbea17c1810c 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1831,7 +1831,8 @@ EXPORT_SYMBOL_GPL(kvm_release_page_clean);
void kvm_release_pfn_clean(kvm_pfn_t pfn)
{
- if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn))
+ if ((!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn)) ||
+ (pfn_valid(pfn) && is_zone_device_page(pfn_to_page(pfn
put_page(pfn_to_page(pfn));
}
EXPORT_SYMBOL_GPL(kvm_release_pfn_clean);
I had the same thought, but I do wonder about the kvm_get_pfn() users,
e.g.,:
hva_to_pfn_remapped():
r = follow_pfn(vma, addr, &pfn);
...
kvm_get_pfn(pfn);
...
We would not take a reference for ZONE_DEVICE, but later drop one
reference via kvm_release_pfn_clean(). IOW, kvm_get_pfn() gets *really*
dangerous to use. I can't tell if this can happen right now.
We do have 3 users of kvm_get_pfn() that we have to audit before this
change. Also, we should add a comment to kvm_get_pfn() that it should
never be used with possible ZONE_DEVICE pages.
Also, we should add a comment to kvm_release_pfn_clean(), describing why
we treat ZONE_DEVICE in a special way here.
We can then
Re: [RFC] errno.h: Provide EFSCORRUPTED for everybody
On Tue, Nov 05, 2019 at 04:15:50PM +0100, David Sterba wrote: > On Sat, Nov 02, 2019 at 08:38:23AM +1100, Dave Chinner wrote: > > On Fri, Nov 01, 2019 at 09:57:31PM +0100, Geert Uytterhoeven wrote: > > > Hi Valdis, > > > > > > On Thu, Oct 31, 2019 at 2:11 AM Valdis Kletnieks > > > wrote: > > > > Three questions: (a) ACK/NAK on this patch, (b) should it be all in one > > > > patch, or one to add to errno.h and 6 patches for 6 filesystems?), and > > > > (c) if one patch, who gets to shepherd it through? > > > > > > > > There's currently 6 filesystems that have the same #define. Move it > > > > into errno.h so it's defined in just one place. > > > > > > > > Signed-off-by: Valdis Kletnieks > > > > > > Thanks for your patch! > > > > > > > --- a/include/uapi/asm-generic/errno.h > > > > +++ b/include/uapi/asm-generic/errno.h > > > > @@ -98,6 +98,7 @@ > > > > #defineEINPROGRESS 115 /* Operation now in progress */ > > > > #defineESTALE 116 /* Stale file handle */ > > > > #defineEUCLEAN 117 /* Structure needs cleaning */ > > > > +#defineEFSCORRUPTEDEUCLEAN > > > > > > I have two questions: > > > a) Why not use EUCLEAN everywhere instead? > > > Having two different names for the same errno complicates grepping. > > > > Because: > > a) EUCLEAN is horrible for code documentation. EFSCORRUPTED > > describes exactly the error being returned and/or checked for. > > > > b) we've used EFSCORRUPTED in XFS since 1993. i.e. it was an > > official, published error value on Irix, and we've kept it > > in the linux code for the past ~20 years because of a) > > > > c) Userspace programs that include filesystem specific > > headers have already been exposed to and use EFSCORRUPTED, > > so we can't remove/change it without breaking userspace. > > > > d) EUCLEAN has a convenient userspace string description > > that is appropriate for filesystem corruption: "Structure > > needs cleaning" is precisely what needs to happen. Repair of > > the filesystem (i.e. recovery to a clean state) is what is > > required to fix the error > > The description is very confusing to users that are also not filesystem > developers. That's a pretty good description of most error messages for modern software packages. Anything that is a non-trivial error requires web searching to understand and diagnose these days. Google knows exactly what you are looking for if you search for "mount structure needs cleaning" or other similar error messages. That means users also know what it means and what they need to do to address it. i.e. it's been around long enough that there's a deep history in internet search engines and question forums like stackexchange. > "Structure needs cleaning" says what needs to be done but > not what happened. Unlike other error codes like "not enough memory", > "IO error" etc. We don't have EBUYMEM / "Buy more memory" instead of > ENOMEM. Message grammar is largely irrelevant. And not unique to EUCLEAN. e.g. EAGAIN = "Try again". > Fuzzing tests and crafted images produce most of the EUCLEAN errors and > in this context "structure needs cleaning" makes even less sense. It's pretty silly to argue that a developer crafting and/or fuzzing corrupted filesystem images is not going to understand what the error message returned when they successfully corrupt a filesystem actually means > > > b) Perhaps both errors should use different values? > > > > That horse bolted to userspace years ago - this is just > > formalising the practice that has spread across multiple linux > > filesystems from XFS over the past ~10 years.. > > EFSCORRUPTED is a appropriate name but to share the number with > EUCLEAN was a terrible decision and now every filesystem has to > suffer and explain to users what the code really means and point > to the the sad story when asked "So why don't you have a separate > code?". Damned if you do, damned if you don't. Back in 2005-2006, XFS developers tried to make EFSCORRUPTED a proper system wide errno (like we had on Irix). The NIH was strong in the linux community back then, and we were largely told to go away as the superior Linux filesystems would never need to report corruption to userspace so we don't need a special errno just because some shitty Irix filesystem port needs it. And so we didn't get a new errno and went with the second choice: precedence set by older unix systems commit 9e1fd551aba7291564d5d6c28948405142d5e2ca Author: Nathan Scott Date: Tue Jun 20 03:49:47 2006 + Map EFSCORRUPTED to an actual error code, not just a made up one (990). Turns out some ye-olde unices used EUCLEAN as Filesystem-needs-cleaning, so now we use that too. Merge of xfs-linux-melb:xfs-kern:26286a by kenmcd. diff --git a/fs/xfs/linux-2.6/xfs_linux.h b/fs/xfs/linux-2.6/xfs_linux.h index 3910afa7..b4083f8c 100644 --- a/fs/xfs/linux-2.6/xfs_linux.h +++
RE: [PATCH v5 2/8] arm64: hyperv: Add hypercall and register access functions
From: Boqun Feng Sent: Sunday, November 3, 2019 8:37 PM
>
> > diff --git a/arch/arm64/Kbuild b/arch/arm64/Kbuild
> > index d646582..2469421 100644
> > --- a/arch/arm64/Kbuild
> > +++ b/arch/arm64/Kbuild
> > @@ -3,4 +3,5 @@ obj-y += kernel/ mm/
> > obj-$(CONFIG_NET) += net/
> > obj-$(CONFIG_KVM) += kvm/
> > obj-$(CONFIG_XEN) += xen/
> > +obj-$(CONFIG_HYPERV) += hyperv/
>
> I did a kernel built with CONFIG_HYPERV=m today, and found out this line
> should be (similar to x86):
>
> +obj-$(subst m,y,$(CONFIG_HYPERV)) += hyperv/
>
> , otherwise, when CONFIG_HYPERV=m, files in arch/arm64/hyperv/ will be
> compiled as obj-m, and symbols defined in those files cannot be
> used by kernel builtin, e.g. hyperv_timer (since CONFIG_HYPERV_TIMER=y
> in this case).
Agreed. I'll fix that in the next version.
>
> A compile/link error I hit today is:
>
> | /home/boqun/linux-arm64/drivers/clocksource/hyperv_timer.c:98: undefined
> reference
> to `hv_set_vpreg'
> | aarch64-linux-gnu-ld:
> /home/boqun/linux-arm64/drivers/clocksource/hyperv_timer.c:98:
> undefined reference to `hv_set_vpreg'
I'm not seeing this error. I'm building natively on an ARM64 system, though
the environment and tools are perhaps a couple of years old. Are you still
able to reproduce the above error? And is it only complaining about
'hv_set_vpreg', or also about similar functions like 'hv_get_vpreg' that
are very parallel?
>
> [...]
>
> Besides, another problem I hit when compiled with CONFIG_HYPERV=m is:
>
> | ERROR: "screen_info" [drivers/hv/hv_vmbus.ko] undefined!
>
> , which can be fixed by the following change.
>
> Regards,
> Boqun
>
> >8
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index d0cf596db82c..8ff557ae5cc6 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
>
> @@ -55,6 +55,7 @@ static __init pteval_t
> create_mapping_protection(efi_memory_desc_t
> *md)
>
> /* we will fill this structure from the stub, so don't put it in .bss */
> struct screen_info screen_info __section(.data);
> +EXPORT_SYMBOL(screen_info);
>
> int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
> {
Agreed. I can reproduce the same problem, and will fix it as you suggest.
Michael
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 03/10] KVM: Prepare kvm_is_reserved_pfn() for PG_reserved changes
On Tue, Nov 5, 2019 at 4:03 PM Sean Christopherson
wrote:
>
> On Tue, Nov 05, 2019 at 03:43:29PM -0800, Dan Williams wrote:
> > On Tue, Nov 5, 2019 at 3:30 PM Dan Williams
> > wrote:
> > >
> > > On Tue, Nov 5, 2019 at 3:13 PM Sean Christopherson
> > > wrote:
> > > >
> > > > On Tue, Nov 05, 2019 at 03:02:40PM -0800, Dan Williams wrote:
> > > > > On Tue, Nov 5, 2019 at 12:31 PM David Hildenbrand
> > > > > wrote:
> > > > > > > The scarier code (for me) is transparent_hugepage_adjust() and
> > > > > > > kvm_mmu_zap_collapsible_spte(), as I don't at all understand the
> > > > > > > interaction between THP and _PAGE_DEVMAP.
> > > > > >
> > > > > > The x86 KVM MMU code is one of the ugliest code I know (sorry, but
> > > > > > it
> > > > > > had to be said :/ ). Luckily, this should be independent of the
> > > > > > PG_reserved thingy AFAIKs.
> > > > >
> > > > > Both transparent_hugepage_adjust() and kvm_mmu_zap_collapsible_spte()
> > > > > are honoring kvm_is_reserved_pfn(), so again I'm missing where the
> > > > > page count gets mismanaged and leads to the reported hang.
> > > >
> > > > When mapping pages into the guest, KVM gets the page via gup(), which
> > > > increments the page count for ZONE_DEVICE pages. But KVM puts the page
> > > > using kvm_release_pfn_clean(), which skips put_page() if PageReserved()
> > > > and so never puts its reference to ZONE_DEVICE pages.
> > >
> > > Oh, yeah, that's busted.
> >
> > Ugh, it's extra busted because every other gup user in the kernel
> > tracks the pages resulting from gup and puts them (put_page()) when
> > they are done. KVM wants to forget about whether it did a gup to get
> > the page and optionally trigger put_page() based purely on the pfn.
> > Outside of VFIO device assignment that needs pages pinned for DMA, why
> > does KVM itself need to pin pages? If pages are pinned over a return
> > to userspace that needs to be a FOLL_LONGTERM gup.
>
> Short answer, KVM pins the page to ensure correctness with respect to the
> primary MMU invalidating the associated host virtual address, e.g. when
> the page is being migrated or unmapped from host userspace.
>
> The main use of gup() is to handle guest page faults and map pages into
> the guest, i.e. into KVM's secondary MMU. KVM uses gup() to both get the
> PFN and to temporarily pin the page. The pin is held just long enough to
> guaranteed that any invalidation via the mmu_notifier will be stalled
> until after KVM finishes installing the page into the secondary MMU, i.e.
> the pin is short-term and not held across a return to userspace or entry
> into the guest. When a subsequent mmu_notifier invalidation occurs, KVM
> pulls the PFN from the secondary MMU and uses that to update accessed
> and dirty bits in the host.
>
> There are a few other KVM flows that eventually call into gup(), but those
> are "traditional" short-term pins and use put_page() directly.
Ok, I was misinterpreting the effect of the bug with what KVM is using
the reference to do.
To your other point:
> But David's proposed fix for the above refcount bug is to omit the patch
> so that KVM no longer treats ZONE_DEVICE pages as reserved. That seems
> like the right thing to do, including for thp_adjust(), e.g. it would
> naturally let KVM use 2mb pages for the guest when a ZONE_DEVICE page is
> mapped with a huge page (2mb or above) in the host. The only hiccup is
> figuring out how to correctly transfer the reference.
That might not be the only hiccup. There's currently no such thing as
huge pages for ZONE_DEVICE, there are huge *mappings* (pmd and pud),
but the result of pfn_to_page() on such a mapping does not yield a
huge 'struct page'. It seems there are other paths in KVM that assume
that more typical page machinery is active like SetPageDirty() based
on kvm_is_reserved_pfn(). While I told David that I did not want to
see more usage of is_zone_device_page(), this patch below (untested)
seems a cleaner path with less surprises:
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 4df0aa6b8e5c..fbea17c1810c 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1831,7 +1831,8 @@ EXPORT_SYMBOL_GPL(kvm_release_page_clean);
void kvm_release_pfn_clean(kvm_pfn_t pfn)
{
- if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn))
+ if ((!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn)) ||
+ (pfn_valid(pfn) && is_zone_device_page(pfn_to_page(pfn
put_page(pfn_to_page(pfn));
}
EXPORT_SYMBOL_GPL(kvm_release_pfn_clean);
This is safe because the reference that KVM took earlier protects the
is_zone_device_page() lookup from racing device teardown. Otherwise,
if KVM does not have a reference it's unsafe, but that's already even
more broken because KVM would be releasing a page that it never
referenced. Every other KVM operation that assumes page allocator
pages would continue to honor kvm_is_reserved_pfn().
___
devel mailing l
Re: [PATCH v1 03/10] KVM: Prepare kvm_is_reserved_pfn() for PG_reserved changes
On Tue, Nov 05, 2019 at 03:43:29PM -0800, Dan Williams wrote: > On Tue, Nov 5, 2019 at 3:30 PM Dan Williams wrote: > > > > On Tue, Nov 5, 2019 at 3:13 PM Sean Christopherson > > wrote: > > > > > > On Tue, Nov 05, 2019 at 03:02:40PM -0800, Dan Williams wrote: > > > > On Tue, Nov 5, 2019 at 12:31 PM David Hildenbrand > > > > wrote: > > > > > > The scarier code (for me) is transparent_hugepage_adjust() and > > > > > > kvm_mmu_zap_collapsible_spte(), as I don't at all understand the > > > > > > interaction between THP and _PAGE_DEVMAP. > > > > > > > > > > The x86 KVM MMU code is one of the ugliest code I know (sorry, but it > > > > > had to be said :/ ). Luckily, this should be independent of the > > > > > PG_reserved thingy AFAIKs. > > > > > > > > Both transparent_hugepage_adjust() and kvm_mmu_zap_collapsible_spte() > > > > are honoring kvm_is_reserved_pfn(), so again I'm missing where the > > > > page count gets mismanaged and leads to the reported hang. > > > > > > When mapping pages into the guest, KVM gets the page via gup(), which > > > increments the page count for ZONE_DEVICE pages. But KVM puts the page > > > using kvm_release_pfn_clean(), which skips put_page() if PageReserved() > > > and so never puts its reference to ZONE_DEVICE pages. > > > > Oh, yeah, that's busted. > > Ugh, it's extra busted because every other gup user in the kernel > tracks the pages resulting from gup and puts them (put_page()) when > they are done. KVM wants to forget about whether it did a gup to get > the page and optionally trigger put_page() based purely on the pfn. > Outside of VFIO device assignment that needs pages pinned for DMA, why > does KVM itself need to pin pages? If pages are pinned over a return > to userspace that needs to be a FOLL_LONGTERM gup. Short answer, KVM pins the page to ensure correctness with respect to the primary MMU invalidating the associated host virtual address, e.g. when the page is being migrated or unmapped from host userspace. The main use of gup() is to handle guest page faults and map pages into the guest, i.e. into KVM's secondary MMU. KVM uses gup() to both get the PFN and to temporarily pin the page. The pin is held just long enough to guaranteed that any invalidation via the mmu_notifier will be stalled until after KVM finishes installing the page into the secondary MMU, i.e. the pin is short-term and not held across a return to userspace or entry into the guest. When a subsequent mmu_notifier invalidation occurs, KVM pulls the PFN from the secondary MMU and uses that to update accessed and dirty bits in the host. There are a few other KVM flows that eventually call into gup(), but those are "traditional" short-term pins and use put_page() directly. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 03/10] KVM: Prepare kvm_is_reserved_pfn() for PG_reserved changes
On Tue, Nov 5, 2019 at 3:30 PM Dan Williams wrote: > > On Tue, Nov 5, 2019 at 3:13 PM Sean Christopherson > wrote: > > > > On Tue, Nov 05, 2019 at 03:02:40PM -0800, Dan Williams wrote: > > > On Tue, Nov 5, 2019 at 12:31 PM David Hildenbrand > > > wrote: > > > > > The scarier code (for me) is transparent_hugepage_adjust() and > > > > > kvm_mmu_zap_collapsible_spte(), as I don't at all understand the > > > > > interaction between THP and _PAGE_DEVMAP. > > > > > > > > The x86 KVM MMU code is one of the ugliest code I know (sorry, but it > > > > had to be said :/ ). Luckily, this should be independent of the > > > > PG_reserved thingy AFAIKs. > > > > > > Both transparent_hugepage_adjust() and kvm_mmu_zap_collapsible_spte() > > > are honoring kvm_is_reserved_pfn(), so again I'm missing where the > > > page count gets mismanaged and leads to the reported hang. > > > > When mapping pages into the guest, KVM gets the page via gup(), which > > increments the page count for ZONE_DEVICE pages. But KVM puts the page > > using kvm_release_pfn_clean(), which skips put_page() if PageReserved() > > and so never puts its reference to ZONE_DEVICE pages. > > Oh, yeah, that's busted. Ugh, it's extra busted because every other gup user in the kernel tracks the pages resulting from gup and puts them (put_page()) when they are done. KVM wants to forget about whether it did a gup to get the page and optionally trigger put_page() based purely on the pfn. Outside of VFIO device assignment that needs pages pinned for DMA, why does KVM itself need to pin pages? If pages are pinned over a return to userspace that needs to be a FOLL_LONGTERM gup. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 03/10] KVM: Prepare kvm_is_reserved_pfn() for PG_reserved changes
On Tue, Nov 05, 2019 at 03:30:00PM -0800, Dan Williams wrote: > On Tue, Nov 5, 2019 at 3:13 PM Sean Christopherson > wrote: > > > > On Tue, Nov 05, 2019 at 03:02:40PM -0800, Dan Williams wrote: > > > On Tue, Nov 5, 2019 at 12:31 PM David Hildenbrand > > > wrote: > > > > > The scarier code (for me) is transparent_hugepage_adjust() and > > > > > kvm_mmu_zap_collapsible_spte(), as I don't at all understand the > > > > > interaction between THP and _PAGE_DEVMAP. > > > > > > > > The x86 KVM MMU code is one of the ugliest code I know (sorry, but it > > > > had to be said :/ ). Luckily, this should be independent of the > > > > PG_reserved thingy AFAIKs. > > > > > > Both transparent_hugepage_adjust() and kvm_mmu_zap_collapsible_spte() > > > are honoring kvm_is_reserved_pfn(), so again I'm missing where the > > > page count gets mismanaged and leads to the reported hang. > > > > When mapping pages into the guest, KVM gets the page via gup(), which > > increments the page count for ZONE_DEVICE pages. But KVM puts the page > > using kvm_release_pfn_clean(), which skips put_page() if PageReserved() > > and so never puts its reference to ZONE_DEVICE pages. > > Oh, yeah, that's busted. > > > My transparent_hugepage_adjust() and kvm_mmu_zap_collapsible_spte() > > comments were for a post-patch/series scenario wheren PageReserved() is > > no longer true for ZONE_DEVICE pages. > > Ah, ok, for that David is preserving kvm_is_reserved_pfn() returning > true for ZONE_DEVICE because pfn_to_online_page() will fail for > ZONE_DEVICE. But David's proposed fix for the above refcount bug is to omit the patch so that KVM no longer treats ZONE_DEVICE pages as reserved. That seems like the right thing to do, including for thp_adjust(), e.g. it would naturally let KVM use 2mb pages for the guest when a ZONE_DEVICE page is mapped with a huge page (2mb or above) in the host. The only hiccup is figuring out how to correctly transfer the reference. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 03/10] KVM: Prepare kvm_is_reserved_pfn() for PG_reserved changes
On Tue, Nov 5, 2019 at 3:13 PM Sean Christopherson wrote: > > On Tue, Nov 05, 2019 at 03:02:40PM -0800, Dan Williams wrote: > > On Tue, Nov 5, 2019 at 12:31 PM David Hildenbrand wrote: > > > > The scarier code (for me) is transparent_hugepage_adjust() and > > > > kvm_mmu_zap_collapsible_spte(), as I don't at all understand the > > > > interaction between THP and _PAGE_DEVMAP. > > > > > > The x86 KVM MMU code is one of the ugliest code I know (sorry, but it > > > had to be said :/ ). Luckily, this should be independent of the > > > PG_reserved thingy AFAIKs. > > > > Both transparent_hugepage_adjust() and kvm_mmu_zap_collapsible_spte() > > are honoring kvm_is_reserved_pfn(), so again I'm missing where the > > page count gets mismanaged and leads to the reported hang. > > When mapping pages into the guest, KVM gets the page via gup(), which > increments the page count for ZONE_DEVICE pages. But KVM puts the page > using kvm_release_pfn_clean(), which skips put_page() if PageReserved() > and so never puts its reference to ZONE_DEVICE pages. Oh, yeah, that's busted. > My transparent_hugepage_adjust() and kvm_mmu_zap_collapsible_spte() > comments were for a post-patch/series scenario wheren PageReserved() is > no longer true for ZONE_DEVICE pages. Ah, ok, for that David is preserving kvm_is_reserved_pfn() returning true for ZONE_DEVICE because pfn_to_online_page() will fail for ZONE_DEVICE. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 03/10] KVM: Prepare kvm_is_reserved_pfn() for PG_reserved changes
On Tue, Nov 05, 2019 at 03:02:40PM -0800, Dan Williams wrote: > On Tue, Nov 5, 2019 at 12:31 PM David Hildenbrand wrote: > > > The scarier code (for me) is transparent_hugepage_adjust() and > > > kvm_mmu_zap_collapsible_spte(), as I don't at all understand the > > > interaction between THP and _PAGE_DEVMAP. > > > > The x86 KVM MMU code is one of the ugliest code I know (sorry, but it > > had to be said :/ ). Luckily, this should be independent of the > > PG_reserved thingy AFAIKs. > > Both transparent_hugepage_adjust() and kvm_mmu_zap_collapsible_spte() > are honoring kvm_is_reserved_pfn(), so again I'm missing where the > page count gets mismanaged and leads to the reported hang. When mapping pages into the guest, KVM gets the page via gup(), which increments the page count for ZONE_DEVICE pages. But KVM puts the page using kvm_release_pfn_clean(), which skips put_page() if PageReserved() and so never puts its reference to ZONE_DEVICE pages. My transparent_hugepage_adjust() and kvm_mmu_zap_collapsible_spte() comments were for a post-patch/series scenario wheren PageReserved() is no longer true for ZONE_DEVICE pages. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 03/10] KVM: Prepare kvm_is_reserved_pfn() for PG_reserved changes
On Tue, Nov 5, 2019 at 12:31 PM David Hildenbrand wrote:
>
> >>> I think I know what's going wrong:
> >>>
> >>> Pages that are pinned via gfn_to_pfn() and friends take a references,
> >>> however are often released via
> >>> kvm_release_pfn_clean()/kvm_release_pfn_dirty()/kvm_release_page_clean()...
> >>>
> >>>
> >>> E.g., in arch/x86/kvm/x86.c:reexecute_instruction()
> >>>
> >>> ...
> >>> pfn = gfn_to_pfn(vcpu->kvm, gpa_to_gfn(gpa));
> >>> ...
> >>> kvm_release_pfn_clean(pfn);
> >>>
> >>>
> >>>
> >>> void kvm_release_pfn_clean(kvm_pfn_t pfn)
> >>> {
> >>> if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn))
> >>> put_page(pfn_to_page(pfn));
> >>> }
> >>>
> >>> This function makes perfect sense as the counterpart for kvm_get_pfn():
> >>>
> >>> void kvm_get_pfn(kvm_pfn_t pfn)
> >>> {
> >>> if (!kvm_is_reserved_pfn(pfn))
> >>> get_page(pfn_to_page(pfn));
> >>> }
> >>>
> >>>
> >>> As all ZONE_DEVICE pages are currently reserved, pages pinned via
> >>> gfn_to_pfn() and friends will often not see a put_page() AFAIKS.
> >
> > Assuming gup() takes a reference for ZONE_DEVICE pages, yes, this is a
> > KVM bug.
>
> Yes, it does take a reference AFAIKs. E.g.,
>
> mm/gup.c:gup_pte_range():
> ...
> if (pte_devmap(pte)) {
> if (unlikely(flags & FOLL_LONGTERM))
> goto pte_unmap;
>
> pgmap = get_dev_pagemap(pte_pfn(pte), pgmap);
> if (unlikely(!pgmap)) {
> undo_dev_pagemap(nr, nr_start, pages);
> goto pte_unmap;
> }
> } else if (pte_special(pte))
> goto pte_unmap;
>
> VM_BUG_ON(!pfn_valid(pte_pfn(pte)));
> page = pte_page(pte);
>
> head = try_get_compound_head(page, 1);
>
> try_get_compound_head() will increment the reference count.
>
> >
> >>> Now, my patch does not change that, the result of
> >>> kvm_is_reserved_pfn(pfn) will be unchanged. A proper fix for that would
> >>> probably be
> >>>
> >>> a) To drop the reference to ZONE_DEVICE pages in gfn_to_pfn() and
> >>> friends, after you successfully pinned the pages. (not sure if that's
> >>> the right thing to do but you're the expert)
> >>>
> >>> b) To not use kvm_release_pfn_clean() and friends on pages that were
> >>> definitely pinned.
> >
> > This is already KVM's intent, i.e. the purpose of the PageReserved() check
> > is simply to avoid putting a non-existent reference. The problem is that
> > KVM assumes pages with PG_reserved set are never pinned, which AFAICT was
> > true when the code was first added.
> >
> >> (talking to myself, sorry)
> >>
> >> Thinking again, dropping this patch from this series could effectively also
> >> fix that issue. E.g., kvm_release_pfn_clean() and friends would always do a
> >> put_page() if "pfn_valid() and !PageReserved()", so after patch 9 also on
> >> ZONDE_DEVICE pages.
> >
> > Yeah, this appears to be the correct fix.
> >
> >> But it would have side effects that might not be desired. E.g.,:
> >>
> >> 1. kvm_pfn_to_page() would also return ZONE_DEVICE pages (might even be the
> >> right thing to do).
> >
> > This should be ok, at least on x86. There are only three users of
> > kvm_pfn_to_page(). Two of those are on allocations that are controlled by
> > KVM and are guaranteed to be vanilla MAP_ANONYMOUS. The third is on guest
> > memory when running a nested guest, and in that case supporting ZONE_DEVICE
> > memory is desirable, i.e. KVM should play nice with a guest that is backed
> > by ZONE_DEVICE memory.
> >
> >> 2. kvm_set_pfn_dirty() would also set ZONE_DEVICE pages dirty (might be
> >> okay)
> >
> > This is ok from a KVM perspective.
>
> What about
>
> void kvm_get_pfn(kvm_pfn_t pfn)
> {
> if (!kvm_is_reserved_pfn(pfn))
> get_page(pfn_to_page(pfn));
> }
>
> Is a pure get_page() sufficient in case of ZONE_DEVICE?
> (asking because of the live references obtained via
> get_dev_pagemap(pte_pfn(pte), pgmap) in mm/gup.c:gup_pte_range()
> somewhat confuse me :) )
It is not sufficient. PTE_DEVMAP is there to tell the gup path "be
careful, this pfn has device-lifetime, make sure the device is pinned
and not actively in the process of dying before pinning any pages
associated with this device".
However, if kvm_get_pfn() is honoring kvm_is_reserved_pfn() that
returns true for ZONE_DEVICE, I'm missing how it is messing up the
reference counts.
> > The scarier code (for me) is transparent_hugepage_adjust() and
> > kvm_mmu_zap_collapsible_spte(), as I don't at all understand the
> > interaction between THP and _PAGE_DEVMAP.
>
> The x86 KVM MMU code is one of the ugliest code I know (sorry, but it
> had to be said :/ ). Luckily, this should be independent of the
> PG_reserved thingy AFAIKs.
Both transparent_hugepage_adjust() and kvm_mmu_zap_collapsible_spte()
are honorin
[PATCH] staging: vchiq: Have vchiu_queue_init() return 0 on success.
It could be confusing to return 1 on success and 0 on error
given the style elswhere.
Signed-off-by: Marcelo Diop-Gonzalez
---
.../staging/vc04_services/interface/vchiq_arm/vchiq_shim.c| 2 +-
.../staging/vc04_services/interface/vchiq_arm/vchiq_util.c| 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c
index 17a4f2c8d8b1..c76d5b2e0701 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c
@@ -579,7 +579,7 @@ static struct shim_service *service_alloc(VCHIQ_INSTANCE_T
instance,
(void)instance;
if (service) {
- if (vchiu_queue_init(&service->queue, 64)) {
+ if (!vchiu_queue_init(&service->queue, 64)) {
service->callback = setup->callback;
service->callback_param = setup->callback_param;
} else {
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_util.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_util.c
index 5e6d3035dc05..644844d88fed 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_util.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_util.c
@@ -24,9 +24,9 @@ int vchiu_queue_init(struct vchiu_queue *queue, int size)
GFP_KERNEL);
if (!queue->storage) {
vchiu_queue_delete(queue);
- return 0;
+ return -ENOMEM;
}
- return 1;
+ return 0;
}
void vchiu_queue_delete(struct vchiu_queue *queue)
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 03/10] KVM: Prepare kvm_is_reserved_pfn() for PG_reserved changes
On Tue, Nov 05, 2019 at 09:30:53PM +0100, David Hildenbrand wrote:
> >>>I think I know what's going wrong:
> >>>
> >>>Pages that are pinned via gfn_to_pfn() and friends take a references,
> >>>however are often released via
> >>>kvm_release_pfn_clean()/kvm_release_pfn_dirty()/kvm_release_page_clean()...
> >>>
> >>>
> >>>E.g., in arch/x86/kvm/x86.c:reexecute_instruction()
> >>>
> >>>...
> >>>pfn = gfn_to_pfn(vcpu->kvm, gpa_to_gfn(gpa));
> >>>...
> >>>kvm_release_pfn_clean(pfn);
> >>>
> >>>
> >>>
> >>>void kvm_release_pfn_clean(kvm_pfn_t pfn)
> >>>{
> >>> if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn))
> >>> put_page(pfn_to_page(pfn));
> >>>}
> >>>
> >>>This function makes perfect sense as the counterpart for kvm_get_pfn():
> >>>
> >>>void kvm_get_pfn(kvm_pfn_t pfn)
> >>>{
> >>> if (!kvm_is_reserved_pfn(pfn))
> >>> get_page(pfn_to_page(pfn));
> >>>}
> >>>
> >>>
> >>>As all ZONE_DEVICE pages are currently reserved, pages pinned via
> >>>gfn_to_pfn() and friends will often not see a put_page() AFAIKS.
> >
> >Assuming gup() takes a reference for ZONE_DEVICE pages, yes, this is a
> >KVM bug.
>
> Yes, it does take a reference AFAIKs. E.g.,
>
> mm/gup.c:gup_pte_range():
> ...
> if (pte_devmap(pte)) {
> if (unlikely(flags & FOLL_LONGTERM))
> goto pte_unmap;
>
> pgmap = get_dev_pagemap(pte_pfn(pte), pgmap);
> if (unlikely(!pgmap)) {
> undo_dev_pagemap(nr, nr_start, pages);
> goto pte_unmap;
> }
> } else if (pte_special(pte))
> goto pte_unmap;
>
> VM_BUG_ON(!pfn_valid(pte_pfn(pte)));
> page = pte_page(pte);
>
> head = try_get_compound_head(page, 1);
>
> try_get_compound_head() will increment the reference count.
Doh, I looked right at that code and somehow didn't connect the dots.
Thanks!
> >>>Now, my patch does not change that, the result of
> >>>kvm_is_reserved_pfn(pfn) will be unchanged. A proper fix for that would
> >>>probably be
> >>>
> >>>a) To drop the reference to ZONE_DEVICE pages in gfn_to_pfn() and
> >>>friends, after you successfully pinned the pages. (not sure if that's
> >>>the right thing to do but you're the expert)
> >>>
> >>>b) To not use kvm_release_pfn_clean() and friends on pages that were
> >>>definitely pinned.
> >
> >This is already KVM's intent, i.e. the purpose of the PageReserved() check
> >is simply to avoid putting a non-existent reference. The problem is that
> >KVM assumes pages with PG_reserved set are never pinned, which AFAICT was
> >true when the code was first added.
> >
> >>(talking to myself, sorry)
> >>
> >>Thinking again, dropping this patch from this series could effectively also
> >>fix that issue. E.g., kvm_release_pfn_clean() and friends would always do a
> >>put_page() if "pfn_valid() and !PageReserved()", so after patch 9 also on
> >>ZONDE_DEVICE pages.
> >
> >Yeah, this appears to be the correct fix.
> >
> >>But it would have side effects that might not be desired. E.g.,:
> >>
> >>1. kvm_pfn_to_page() would also return ZONE_DEVICE pages (might even be the
> >>right thing to do).
> >
> >This should be ok, at least on x86. There are only three users of
> >kvm_pfn_to_page(). Two of those are on allocations that are controlled by
> >KVM and are guaranteed to be vanilla MAP_ANONYMOUS. The third is on guest
> >memory when running a nested guest, and in that case supporting ZONE_DEVICE
> >memory is desirable, i.e. KVM should play nice with a guest that is backed
> >by ZONE_DEVICE memory.
> >
> >>2. kvm_set_pfn_dirty() would also set ZONE_DEVICE pages dirty (might be
> >>okay)
> >
> >This is ok from a KVM perspective.
>
> What about
>
> void kvm_get_pfn(kvm_pfn_t pfn)
> {
> if (!kvm_is_reserved_pfn(pfn))
> get_page(pfn_to_page(pfn));
> }
>
> Is a pure get_page() sufficient in case of ZONE_DEVICE?
> (asking because of the live references obtained via
> get_dev_pagemap(pte_pfn(pte), pgmap) in mm/gup.c:gup_pte_range() somewhat
> confuse me :) )
This ties into my concern with thp_adjust(). On x86, kvm_get_pfn() is
only used in two flows, to manually get a ref for VM_IO/VM_PFNMAP pages
and to switch the ref when mapping a non-hugetlbfs compound page, i.e. a
THP.
I assume VM_IO and PFNMAP can't apply to ZONE_DEVICE pages.
In the thp_adjust() case, when a THP is encountered and the original PFN
is for a non-PG_head page, KVM transfers the reference to the associated
PG_head page[*] and maps the associated 2mb chunk/page. This is where KVM
uses kvm_get_pfn() and could run afoul of the get_dev_pagemap() refcounts.
[*] Technically I don't think it's guaranteed to be a PG_head, e.g. if the
THP is a 1gb page, as KVM currently only maps THP as 2mb pages. But
the idea is the same, transfer the refcount the PFN that's actually
going into
Re: [PATCH v2 01/10] staging: exfat: Clean up return codes - FFS_FORMATERR
On Tue, 05 Nov 2019 18:05:15 +0100, Greg Kroah-Hartman said:
> This patch breaks the build:
>
> drivers/staging/exfat/exfat_super.c: In function âffsMountVolâ:
> drivers/staging/exfat/exfat_super.c:387:9: error: âFFS_FORMATERRâ
> undeclared
(first use in this function)
> 387 | ret = FFS_FORMATERR;
> | ^
>
>
> Did you test-build this thing?
Yes.
And in my tree, that section of code has:
385 /* check the validity of PBR */
386 if (GET16_A(p_pbr->signature) != PBR_SIGNATURE) {
387 brelse(tmp_bh);
388 bdev_close(sb);
389 ret = -EFSCORRUPTED;
390 goto out;
391 }
but 'git blame' says that was changed in patch 02/10 not 01/10, most likely
due to a miscue with 'git add'.
Will fix and resend.
pgpa9bGcfbHHf.pgp
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 03/10] KVM: Prepare kvm_is_reserved_pfn() for PG_reserved changes
I think I know what's going wrong:
Pages that are pinned via gfn_to_pfn() and friends take a references,
however are often released via
kvm_release_pfn_clean()/kvm_release_pfn_dirty()/kvm_release_page_clean()...
E.g., in arch/x86/kvm/x86.c:reexecute_instruction()
...
pfn = gfn_to_pfn(vcpu->kvm, gpa_to_gfn(gpa));
...
kvm_release_pfn_clean(pfn);
void kvm_release_pfn_clean(kvm_pfn_t pfn)
{
if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn))
put_page(pfn_to_page(pfn));
}
This function makes perfect sense as the counterpart for kvm_get_pfn():
void kvm_get_pfn(kvm_pfn_t pfn)
{
if (!kvm_is_reserved_pfn(pfn))
get_page(pfn_to_page(pfn));
}
As all ZONE_DEVICE pages are currently reserved, pages pinned via
gfn_to_pfn() and friends will often not see a put_page() AFAIKS.
Assuming gup() takes a reference for ZONE_DEVICE pages, yes, this is a
KVM bug.
Yes, it does take a reference AFAIKs. E.g.,
mm/gup.c:gup_pte_range():
...
if (pte_devmap(pte)) {
if (unlikely(flags & FOLL_LONGTERM))
goto pte_unmap;
pgmap = get_dev_pagemap(pte_pfn(pte), pgmap);
if (unlikely(!pgmap)) {
undo_dev_pagemap(nr, nr_start, pages);
goto pte_unmap;
}
} else if (pte_special(pte))
goto pte_unmap;
VM_BUG_ON(!pfn_valid(pte_pfn(pte)));
page = pte_page(pte);
head = try_get_compound_head(page, 1);
try_get_compound_head() will increment the reference count.
Now, my patch does not change that, the result of
kvm_is_reserved_pfn(pfn) will be unchanged. A proper fix for that would
probably be
a) To drop the reference to ZONE_DEVICE pages in gfn_to_pfn() and
friends, after you successfully pinned the pages. (not sure if that's
the right thing to do but you're the expert)
b) To not use kvm_release_pfn_clean() and friends on pages that were
definitely pinned.
This is already KVM's intent, i.e. the purpose of the PageReserved() check
is simply to avoid putting a non-existent reference. The problem is that
KVM assumes pages with PG_reserved set are never pinned, which AFAICT was
true when the code was first added.
(talking to myself, sorry)
Thinking again, dropping this patch from this series could effectively also
fix that issue. E.g., kvm_release_pfn_clean() and friends would always do a
put_page() if "pfn_valid() and !PageReserved()", so after patch 9 also on
ZONDE_DEVICE pages.
Yeah, this appears to be the correct fix.
But it would have side effects that might not be desired. E.g.,:
1. kvm_pfn_to_page() would also return ZONE_DEVICE pages (might even be the
right thing to do).
This should be ok, at least on x86. There are only three users of
kvm_pfn_to_page(). Two of those are on allocations that are controlled by
KVM and are guaranteed to be vanilla MAP_ANONYMOUS. The third is on guest
memory when running a nested guest, and in that case supporting ZONE_DEVICE
memory is desirable, i.e. KVM should play nice with a guest that is backed
by ZONE_DEVICE memory.
2. kvm_set_pfn_dirty() would also set ZONE_DEVICE pages dirty (might be
okay)
This is ok from a KVM perspective.
What about
void kvm_get_pfn(kvm_pfn_t pfn)
{
if (!kvm_is_reserved_pfn(pfn))
get_page(pfn_to_page(pfn));
}
Is a pure get_page() sufficient in case of ZONE_DEVICE?
(asking because of the live references obtained via
get_dev_pagemap(pte_pfn(pte), pgmap) in mm/gup.c:gup_pte_range()
somewhat confuse me :) )
The scarier code (for me) is transparent_hugepage_adjust() and
kvm_mmu_zap_collapsible_spte(), as I don't at all understand the
interaction between THP and _PAGE_DEVMAP.
The x86 KVM MMU code is one of the ugliest code I know (sorry, but it
had to be said :/ ). Luckily, this should be independent of the
PG_reserved thingy AFAIKs.
--
Thanks,
David / dhildenb
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] dt-bindings: iio: accel: add binding documentation for ADIS16240
On Thu, Oct 31, 2019 at 09:03:01PM -0300, Rodrigo Carvalho wrote:
> This patch add device tree binding documentation for ADIS16240.
>
> Signed-off-by: Rodrigo Ribeiro Carvalho
> ---
> V3:
>- Remove spi-cpol and spi-cpha field. They don't seem necessary
Not necessary to document or use? The latter requires the former.
If your device only supports one timing mode, then you don't need them
because it should be implied and the driver can just tell the SPI
subsystem what mode it requires. If the device can support multiple
timing modes, then you should document that you are using the
properties.
> .../bindings/iio/accel/adi,adis16240.yaml | 51 +++
> 1 file changed, 51 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> new file mode 100644
> index ..9a4cd12c4818
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/accel/adi,adis16240.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ADIS16240 Programmable Impact Sensor and Recorder driver
> +
> +maintainers:
> + - Alexandru Ardelean
> +
> +description: |
> + ADIS16240 Programmable Impact Sensor and Recorder driver that supports
> + SPI interface.
> +https://www.analog.com/en/products/adis16240.html
> +
> +properties:
> + compatible:
> +enum:
> + - adi,adis16240
> +
> + reg:
> +maxItems: 1
> +
> + interrupts:
> +maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> +
> +examples:
> + - |
> +#include
> +#include
> +spi0 {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +
> +/* Example for a SPI device node */
> +accelerometer@0 {
> +compatible = "adi,adis16240";
> +reg = <0>;
> +spi-max-frequency = <250>;
> +spi-cpol;
> +spi-cpha;
> +interrupt-parent = <&gpio0>;
> +interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +};
> +};
> --
> 2.23.0
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] hp100: remove set but not used variable val
On Tue, 2019-11-05 at 16:50 +0100, Greg KH wrote: > On Tue, Nov 05, 2019 at 10:36:59PM +0800, Chen Wandun wrote: > > From: Chenwandun > > > > Fixes gcc '-Wunused-but-set-variable' warning: > > > > drivers/staging/hp/hp100.c: In function hp100_start_xmit: > > drivers/staging/hp/hp100.c:1629:10: warning: variable val set but not used > > [-Wunused-but-set-variable] > > > > Signed-off-by: Chenwandun > > I need a "full" name here, like the one on your email "From:" line. You also need the submitter to run checkpatch on the patch and not just the file. WARNING: drivers/staging/hp/hp100.c is marked as 'obsolete' in the MAINTAINERS hierarchy. No unnecessary modifications please. WARNING: drivers/staging/hp/hp100.c is marked as 'obsolete' in the MAINTAINERS hierarchy. No unnecessary modifications please. total: 0 errors, 2 warnings, 0 checks, 18 lines checked > > diff --git a/drivers/staging/hp/hp100.c b/drivers/staging/hp/hp100.c [] > > @@ -1626,7 +1626,9 @@ static netdev_tx_t hp100_start_xmit(struct sk_buff > > *skb, > > unsigned long flags; > > int i, ok_flag; > > int ioaddr = dev->base_addr; > > +#ifdef HP100_DEBUG_TX > > u_short val; > > +#endif > > #ifdefs are not ok in .c code, sorry. > > > struct hp100_private *lp = netdev_priv(dev); > > > > #ifdef HP100_DEBUG_B > > @@ -1695,7 +1697,9 @@ static netdev_tx_t hp100_start_xmit(struct sk_buff > > *skb, > > > > spin_lock_irqsave(&lp->lock, flags); > > hp100_ints_off(); > > +#ifdef HP100_DEBUG_TX > > val = hp100_inw(IRQ_STATUS); > > Are you sure that this doesn't actually change the hardware in some way? If anyone still _has_ the hardware, please let me know. I have the only VG test equipment I know of in a box somewhere in my basement and it's yours for the asking and the postage. It hasn't been powered on in 25 years, no guarantees... ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH][next] staging: vboxsf: fix dereference of pointer dentry before it is null checked
From: Colin Ian King
Currently the pointer dentry is being dereferenced before it is
being null checked. Fix this by only dereferencing dentry once
we know it is not null.
Addresses-Coverity: ("Dereference before null check")
Fixes: df4028658f9d ("staging: Add VirtualBox guest shared folder (vboxsf)
support")
Signed-off-by: Colin Ian King
---
drivers/staging/vboxsf/utils.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/vboxsf/utils.c b/drivers/staging/vboxsf/utils.c
index 1870b69c824e..34a49e6f74fc 100644
--- a/drivers/staging/vboxsf/utils.c
+++ b/drivers/staging/vboxsf/utils.c
@@ -174,7 +174,7 @@ int vboxsf_stat_dentry(struct dentry *dentry, struct
shfl_fsobjinfo *info)
int vboxsf_inode_revalidate(struct dentry *dentry)
{
- struct vboxsf_sbi *sbi = VBOXSF_SBI(dentry->d_sb);
+ struct vboxsf_sbi *sbi;
struct vboxsf_inode *sf_i;
struct shfl_fsobjinfo info;
struct timespec64 prev_mtime;
@@ -187,6 +187,7 @@ int vboxsf_inode_revalidate(struct dentry *dentry)
inode = d_inode(dentry);
prev_mtime = inode->i_mtime;
sf_i = VBOXSF_I(inode);
+ sbi = VBOXSF_SBI(dentry->d_sb);
if (!sf_i->force_restat) {
if (time_before(jiffies, dentry->d_time + sbi->o.ttl))
return 0;
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 01/10] staging: exfat: Clean up return codes - FFS_FORMATERR
On Sun, Nov 03, 2019 at 08:44:57PM -0500, Valdis Kletnieks wrote: > Convert FFS_FORMATERR to -EFSCORRUPTED > > Signed-off-by: Valdis Kletnieks > --- > drivers/staging/exfat/exfat.h | 3 ++- > drivers/staging/exfat/exfat_core.c | 8 > 2 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/staging/exfat/exfat.h b/drivers/staging/exfat/exfat.h > index acb73f47a253..4f9ba235d967 100644 > --- a/drivers/staging/exfat/exfat.h > +++ b/drivers/staging/exfat/exfat.h > @@ -30,6 +30,8 @@ > #undef DEBUG > #endif > > +#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */ > + > #define DENTRY_SIZE 32 /* dir entry size */ > #define DENTRY_SIZE_BITS 5 > > @@ -209,7 +211,6 @@ static inline u16 get_row_index(u16 i) > /* return values */ > #define FFS_SUCCESS 0 > #define FFS_MEDIAERR1 > -#define FFS_FORMATERR 2 > #define FFS_MOUNTED 3 > #define FFS_NOTMOUNTED 4 > #define FFS_ALIGNMENTERR5 > diff --git a/drivers/staging/exfat/exfat_core.c > b/drivers/staging/exfat/exfat_core.c > index b23fbf3ebaa5..e90b54a17150 100644 > --- a/drivers/staging/exfat/exfat_core.c > +++ b/drivers/staging/exfat/exfat_core.c > @@ -573,7 +573,7 @@ s32 load_alloc_bitmap(struct super_block *sb) > return FFS_MEDIAERR; > } > > - return FFS_FORMATERR; > + return -EFSCORRUPTED; > } > > void free_alloc_bitmap(struct super_block *sb) > @@ -3016,7 +3016,7 @@ s32 fat16_mount(struct super_block *sb, struct > pbr_sector_t *p_pbr) > struct bd_info_t *p_bd = &(EXFAT_SB(sb)->bd_info); > > if (p_bpb->num_fats == 0) > - return FFS_FORMATERR; > + return -EFSCORRUPTED; > > num_root_sectors = GET16(p_bpb->num_root_entries) << DENTRY_SIZE_BITS; > num_root_sectors = ((num_root_sectors - 1) >> > @@ -3078,7 +3078,7 @@ s32 fat32_mount(struct super_block *sb, struct > pbr_sector_t *p_pbr) > struct bd_info_t *p_bd = &(EXFAT_SB(sb)->bd_info); > > if (p_bpb->num_fats == 0) > - return FFS_FORMATERR; > + return -EFSCORRUPTED; > > p_fs->sectors_per_clu = p_bpb->sectors_per_clu; > p_fs->sectors_per_clu_bits = ilog2(p_bpb->sectors_per_clu); > @@ -3157,7 +3157,7 @@ s32 exfat_mount(struct super_block *sb, struct > pbr_sector_t *p_pbr) > struct bd_info_t *p_bd = &(EXFAT_SB(sb)->bd_info); > > if (p_bpb->num_fats == 0) > - return FFS_FORMATERR; > + return -EFSCORRUPTED; > > p_fs->sectors_per_clu = 1 << p_bpb->sectors_per_clu_bits; > p_fs->sectors_per_clu_bits = p_bpb->sectors_per_clu_bits; This patch breaks the build: drivers/staging/exfat/exfat_super.c: In function ‘ffsMountVol’: drivers/staging/exfat/exfat_super.c:387:9: error: ‘FFS_FORMATERR’ undeclared (first use in this function) 387 | ret = FFS_FORMATERR; | ^ Did you test-build this thing? thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 00/10] staging: exfat: Clean up return codes, revisited
On Sun, Nov 03, 2019 at 08:44:56PM -0500, Valdis Kletnieks wrote: > The rest of the conversion from internal error numbers to the > standard values used in the rest of the kernel. > > Patch 10/10 is logically separate, merging multiple #defines > into one place in errno.h. It's included in the series because > it depends on patch 1/10. > > Valdis Kletnieks (10): > staging: exfat: Clean up return codes - FFS_FORMATERR > staging: exfat: Clean up return codes - FFS_MEDIAERR > staging: exfat: Clean up return codes - FFS_EOF > staging: exfat: Clean up return codes - FFS_INVALIDFID > staging: exfat: Clean up return codes - FFS_ERROR > staging: exfat: Clean up return codes - remove unused codes > staging: exfat: Clean up return codes - FFS_SUCCESS > staging: exfat: Collapse redundant return code translations > staging: exfat: Correct return code > errno.h: Provide EFSCORRUPTED for everybody You forgot to say what changed from v1? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 09/10] mm/memory_hotplug: Don't mark pages PG_reserved when initializing the memmap
On 11/5/19 5:18 AM, David Hildenbrand wrote:
> On 04.11.19 23:44, Boris Ostrovsky wrote:
>> On 10/24/19 8:09 AM, David Hildenbrand wrote:
>>> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
>>> index 4f2e78a5e4db..af69f057913a 100644
>>> --- a/drivers/xen/balloon.c
>>> +++ b/drivers/xen/balloon.c
>>> @@ -374,6 +374,13 @@ static void xen_online_page(struct page *page,
>>> unsigned int order)
>>> mutex_lock(&balloon_mutex);
>>> for (i = 0; i < size; i++) {
>>> p = pfn_to_page(start_pfn + i);
>>> + /*
>>> + * TODO: The core used to mark the pages reserved. Most
>>> probably
>>> + * we can stop doing that now. However, especially
>>> + * alloc_xenballooned_pages() left PG_reserved set
>>> + * on pages that can get mapped to user space.
>>> + */
>>> + __SetPageReserved(p);
>>
>> I suspect this is not needed. Pages can get into balloon either from
>> here or from non-hotplug path (e.g. decrease_reservation()) and so when
>> we get a page from the balloon we would get a random page that may or
>> may not have Reserved bit set.
>
> Yeah, I also think it is fine. If you agree, I'll drop this hunk and
> add details to the patch description.
>
>
Yes, let's do that. Thanks.
-boris
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 03/10] KVM: Prepare kvm_is_reserved_pfn() for PG_reserved changes
On Tue, Nov 05, 2019 at 11:02:46AM +0100, David Hildenbrand wrote:
> On 05.11.19 10:49, David Hildenbrand wrote:
> >On 05.11.19 10:17, David Hildenbrand wrote:
> >>On 05.11.19 05:38, Dan Williams wrote:
> >>>On Thu, Oct 24, 2019 at 5:11 AM David Hildenbrand wrote:
>
> Right now, ZONE_DEVICE memory is always set PG_reserved. We want to
> change that.
>
> KVM has this weird use case that you can map anything from /dev/mem
> into the guest. pfn_valid() is not a reliable check whether the memmap
> was initialized and can be touched. pfn_to_online_page() makes sure
> that we have an initialized memmap (and don't have ZONE_DEVICE memory).
>
> Rewrite kvm_is_reserved_pfn() to make sure the function produces the
> same result once we stop setting ZONE_DEVICE pages PG_reserved.
>
> Cc: Paolo Bonzini
> Cc: "Radim Krčmář"
> Cc: Michal Hocko
> Cc: Dan Williams
> Cc: KarimAllah Ahmed
> Signed-off-by: David Hildenbrand
> ---
> virt/kvm/kvm_main.c | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index e9eb666eb6e8..9d18cc67d124 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -151,9 +151,15 @@ __weak int
> kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
>
> bool kvm_is_reserved_pfn(kvm_pfn_t pfn)
> {
> - if (pfn_valid(pfn))
> - return PageReserved(pfn_to_page(pfn));
> + struct page *page = pfn_to_online_page(pfn);
>
> + /*
> +* We treat any pages that are not online (not managed by the
> buddy)
> +* as reserved - this includes ZONE_DEVICE pages and pages without
> +* a memmap (e.g., mapped via /dev/mem).
> +*/
> + if (page)
> + return PageReserved(page);
> return true;
> }
> >>>
> >>>So after this all the pfn_valid() usage in kvm_main.c is replaced with
> >>>pfn_to_online_page()? Looks correct to me.
> >>>
> >>>However, I'm worried that kvm is taking reference on ZONE_DEVICE pages
> >>>through some other path resulting in this:
> >>>
> >>>
> >>> https://lore.kernel.org/linux-nvdimm/20190919154708.ga24...@angband.pl/
> >>>
> >>>I'll see if this patch set modulates or maintains that failure mode.
> >>>
> >>
> >>I'd assume that the behavior is unchanged. Ithink we get a reference to
> >>these ZONE_DEVICE pages via __get_user_pages_fast() and friends in
> >>hva_to_pfn_fast() and friends in virt/kvm/kvm_main.c
> >>
> >
> >I think I know what's going wrong:
> >
> >Pages that are pinned via gfn_to_pfn() and friends take a references,
> >however are often released via
> >kvm_release_pfn_clean()/kvm_release_pfn_dirty()/kvm_release_page_clean()...
> >
> >
> >E.g., in arch/x86/kvm/x86.c:reexecute_instruction()
> >
> >...
> >pfn = gfn_to_pfn(vcpu->kvm, gpa_to_gfn(gpa));
> >...
> >kvm_release_pfn_clean(pfn);
> >
> >
> >
> >void kvm_release_pfn_clean(kvm_pfn_t pfn)
> >{
> > if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn))
> > put_page(pfn_to_page(pfn));
> >}
> >
> >This function makes perfect sense as the counterpart for kvm_get_pfn():
> >
> >void kvm_get_pfn(kvm_pfn_t pfn)
> >{
> > if (!kvm_is_reserved_pfn(pfn))
> > get_page(pfn_to_page(pfn));
> >}
> >
> >
> >As all ZONE_DEVICE pages are currently reserved, pages pinned via
> >gfn_to_pfn() and friends will often not see a put_page() AFAIKS.
Assuming gup() takes a reference for ZONE_DEVICE pages, yes, this is a
KVM bug.
> >Now, my patch does not change that, the result of
> >kvm_is_reserved_pfn(pfn) will be unchanged. A proper fix for that would
> >probably be
> >
> >a) To drop the reference to ZONE_DEVICE pages in gfn_to_pfn() and
> >friends, after you successfully pinned the pages. (not sure if that's
> >the right thing to do but you're the expert)
> >
> >b) To not use kvm_release_pfn_clean() and friends on pages that were
> >definitely pinned.
This is already KVM's intent, i.e. the purpose of the PageReserved() check
is simply to avoid putting a non-existent reference. The problem is that
KVM assumes pages with PG_reserved set are never pinned, which AFAICT was
true when the code was first added.
> (talking to myself, sorry)
>
> Thinking again, dropping this patch from this series could effectively also
> fix that issue. E.g., kvm_release_pfn_clean() and friends would always do a
> put_page() if "pfn_valid() and !PageReserved()", so after patch 9 also on
> ZONDE_DEVICE pages.
Yeah, this appears to be the correct fix.
> But it would have side effects that might not be desired. E.g.,:
>
> 1. kvm_pfn_to_page() would also return ZONE_DEVICE pages (might even be the
> right thing to do).
This should be ok, at least on x86. There are only three users of
kvm_pfn_to_page(). Two of those are on allocations that are contro
Re: [PATCH v2] hp100: remove set but not used variable val
On Tue, Nov 05, 2019 at 10:36:59PM +0800, Chen Wandun wrote: > From: Chenwandun > > Fixes gcc '-Wunused-but-set-variable' warning: > > drivers/staging/hp/hp100.c: In function hp100_start_xmit: > drivers/staging/hp/hp100.c:1629:10: warning: variable val set but not used > [-Wunused-but-set-variable] > > Signed-off-by: Chenwandun I need a "full" name here, like the one on your email "From:" line. > --- > drivers/staging/hp/hp100.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/staging/hp/hp100.c b/drivers/staging/hp/hp100.c > index 6ec78f5..6fc7733 100644 > --- a/drivers/staging/hp/hp100.c > +++ b/drivers/staging/hp/hp100.c > @@ -1626,7 +1626,9 @@ static netdev_tx_t hp100_start_xmit(struct sk_buff *skb, > unsigned long flags; > int i, ok_flag; > int ioaddr = dev->base_addr; > +#ifdef HP100_DEBUG_TX > u_short val; > +#endif #ifdefs are not ok in .c code, sorry. > struct hp100_private *lp = netdev_priv(dev); > > #ifdef HP100_DEBUG_B > @@ -1695,7 +1697,9 @@ static netdev_tx_t hp100_start_xmit(struct sk_buff *skb, > > spin_lock_irqsave(&lp->lock, flags); > hp100_ints_off(); > +#ifdef HP100_DEBUG_TX > val = hp100_inw(IRQ_STATUS); Are you sure that this doesn't actually change the hardware in some way? thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC] errno.h: Provide EFSCORRUPTED for everybody
On Sat, Nov 02, 2019 at 08:38:23AM +1100, Dave Chinner wrote: > On Fri, Nov 01, 2019 at 09:57:31PM +0100, Geert Uytterhoeven wrote: > > Hi Valdis, > > > > On Thu, Oct 31, 2019 at 2:11 AM Valdis Kletnieks > > wrote: > > > Three questions: (a) ACK/NAK on this patch, (b) should it be all in one > > > patch, or one to add to errno.h and 6 patches for 6 filesystems?), and > > > (c) if one patch, who gets to shepherd it through? > > > > > > There's currently 6 filesystems that have the same #define. Move it > > > into errno.h so it's defined in just one place. > > > > > > Signed-off-by: Valdis Kletnieks > > > > Thanks for your patch! > > > > > --- a/include/uapi/asm-generic/errno.h > > > +++ b/include/uapi/asm-generic/errno.h > > > @@ -98,6 +98,7 @@ > > > #defineEINPROGRESS 115 /* Operation now in progress */ > > > #defineESTALE 116 /* Stale file handle */ > > > #defineEUCLEAN 117 /* Structure needs cleaning */ > > > +#defineEFSCORRUPTEDEUCLEAN > > > > I have two questions: > > a) Why not use EUCLEAN everywhere instead? > > Having two different names for the same errno complicates grepping. > > Because: > a) EUCLEAN is horrible for code documentation. EFSCORRUPTED > describes exactly the error being returned and/or checked for. > > b) we've used EFSCORRUPTED in XFS since 1993. i.e. it was an > official, published error value on Irix, and we've kept it > in the linux code for the past ~20 years because of a) > > c) Userspace programs that include filesystem specific > headers have already been exposed to and use EFSCORRUPTED, > so we can't remove/change it without breaking userspace. > > d) EUCLEAN has a convenient userspace string description > that is appropriate for filesystem corruption: "Structure > needs cleaning" is precisely what needs to happen. Repair of > the filesystem (i.e. recovery to a clean state) is what is > required to fix the error The description is very confusing to users that are also not filesystem developers. "Structure needs cleaning" says what needs to be done but not what happened. Unlike other error codes like "not enough memory", "IO error" etc. We don't have EBUYMEM / "Buy more memory" instead of ENOMEM. Fuzzing tests and crafted images produce most of the EUCLEAN errors and in this context "structure needs cleaning" makes even less sense. > > b) Perhaps both errors should use different values? > > That horse bolted to userspace years ago - this is just formalising > the practice that has spread across multiple linux filesystems from > XFS over the past ~10 years.. EFSCORRUPTED is a appropriate name but to share the number with EUCLEAN was a terrible decision and now every filesystem has to suffer and explain to users what the code really means and point to the the sad story when asked "So why don't you have a separate code?". I wonder what userspace package really depends on the value, that would eg. change code flow. Uncommon error values usually lead to a message and exit. Debian code search shows only jython, e2fsprogs, xfsprogs, python2.7, pypy, linux, partclone using EFSCORRUPTED. So 2 of them are filesystem packages that can handle that, python/jython only defines the value for IRIX platform. The rest is linux kernel, not relevant. So please give me one example where userspace will break. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8192e: fix potential use after free
On Tue, Nov 05, 2019 at 10:49:11PM +0800, Pan Bian wrote: > The variable skb is released via kfree_skb() when the return value of > _rtl92e_tx is not zero. However, after that, skb is accessed again to > read its length, which may result in a use after free bug. This patch > fixes the bug by moving the release operation to where skb is never > used later. > > Signed-off-by: Pan Bian Reviewed-by: Dan Carpenter regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rtl8192e: fix potential use after free
The variable skb is released via kfree_skb() when the return value of
_rtl92e_tx is not zero. However, after that, skb is accessed again to
read its length, which may result in a use after free bug. This patch
fixes the bug by moving the release operation to where skb is never
used later.
Signed-off-by: Pan Bian
---
drivers/staging/rtl8192e/rtl8192e/rtl_core.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_core.c
b/drivers/staging/rtl8192e/rtl8192e/rtl_core.c
index f932cb15e4e5..cdcb22f96ed9 100644
--- a/drivers/staging/rtl8192e/rtl8192e/rtl_core.c
+++ b/drivers/staging/rtl8192e/rtl8192e/rtl_core.c
@@ -1616,14 +1616,15 @@ static void _rtl92e_hard_data_xmit(struct sk_buff *skb,
struct net_device *dev,
memcpy((unsigned char *)(skb->cb), &dev, sizeof(dev));
skb_push(skb, priv->rtllib->tx_headroom);
ret = _rtl92e_tx(dev, skb);
- if (ret != 0)
- kfree_skb(skb);
if (queue_index != MGNT_QUEUE) {
priv->rtllib->stats.tx_bytes += (skb->len -
priv->rtllib->tx_headroom);
priv->rtllib->stats.tx_packets++;
}
+
+ if (ret != 0)
+ kfree_skb(skb);
}
static int _rtl92e_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] hp100: remove set but not used variable val
On Tue, Nov 05, 2019 at 10:36:59PM +0800, Chen Wandun wrote: > From: Chenwandun > > Fixes gcc '-Wunused-but-set-variable' warning: > > drivers/staging/hp/hp100.c: In function hp100_start_xmit: > drivers/staging/hp/hp100.c:1629:10: warning: variable val set but not used > [-Wunused-but-set-variable] > > Signed-off-by: Chenwandun > --- You should explain why you are sending a v2 under the --- cut off line. The v1 was the correct patch, but this driver is going to be deleted soon so I don't think we are accepting this sort of cleanup. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] hp100: remove set but not used variable val
From: Chenwandun Fixes gcc '-Wunused-but-set-variable' warning: drivers/staging/hp/hp100.c: In function hp100_start_xmit: drivers/staging/hp/hp100.c:1629:10: warning: variable val set but not used [-Wunused-but-set-variable] Signed-off-by: Chenwandun --- drivers/staging/hp/hp100.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/staging/hp/hp100.c b/drivers/staging/hp/hp100.c index 6ec78f5..6fc7733 100644 --- a/drivers/staging/hp/hp100.c +++ b/drivers/staging/hp/hp100.c @@ -1626,7 +1626,9 @@ static netdev_tx_t hp100_start_xmit(struct sk_buff *skb, unsigned long flags; int i, ok_flag; int ioaddr = dev->base_addr; +#ifdef HP100_DEBUG_TX u_short val; +#endif struct hp100_private *lp = netdev_priv(dev); #ifdef HP100_DEBUG_B @@ -1695,7 +1697,9 @@ static netdev_tx_t hp100_start_xmit(struct sk_buff *skb, spin_lock_irqsave(&lp->lock, flags); hp100_ints_off(); +#ifdef HP100_DEBUG_TX val = hp100_inw(IRQ_STATUS); +#endif /* Ack / clear the interrupt TX_COMPLETE interrupt - this interrupt is set * when the current packet being transmitted on the wire is completed. */ hp100_outw(HP100_TX_COMPLETE, IRQ_STATUS); -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] hp100: remove set but not used variable val
From: Chenwandun
Fixes gcc '-Wunused-but-set-variable' warning:
drivers/staging/hp/hp100.c: In function hp100_start_xmit:
drivers/staging/hp/hp100.c:1629:10: warning: variable val set but not used
[-Wunused-but-set-variable]
Signed-off-by: Chenwandun
---
drivers/staging/hp/hp100.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/staging/hp/hp100.c b/drivers/staging/hp/hp100.c
index 6ec78f5..7e91737 100644
--- a/drivers/staging/hp/hp100.c
+++ b/drivers/staging/hp/hp100.c
@@ -1626,7 +1626,6 @@ static netdev_tx_t hp100_start_xmit(struct sk_buff *skb,
unsigned long flags;
int i, ok_flag;
int ioaddr = dev->base_addr;
- u_short val;
struct hp100_private *lp = netdev_priv(dev);
#ifdef HP100_DEBUG_B
@@ -1695,13 +1694,12 @@ static netdev_tx_t hp100_start_xmit(struct sk_buff *skb,
spin_lock_irqsave(&lp->lock, flags);
hp100_ints_off();
- val = hp100_inw(IRQ_STATUS);
/* Ack / clear the interrupt TX_COMPLETE interrupt - this interrupt is
set
* when the current packet being transmitted on the wire is completed.
*/
hp100_outw(HP100_TX_COMPLETE, IRQ_STATUS);
#ifdef HP100_DEBUG_TX
printk("hp100: %s: start_xmit: irq_status=0x%.4x, irqmask=0x%.4x,
len=%d\n",
- dev->name, val, hp100_inw(IRQ_MASK), (int) skb->len);
+ dev->name, hp100_inw(IRQ_STATUS), hp100_inw(IRQ_MASK),
(int) skb->len);
#endif
ok_flag = skb->len >= HP100_MIN_PACKET_SIZE;
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rts5208: rewrite macro with GNU extension __auto_type
On Mon, 4 Nov 2019, Greg KH wrote:
> On Mon, Nov 04, 2019 at 04:44:00PM +, Jules Irenge wrote:
> > Rewrite macro function with GNU extension __auto_type
> > to remove issue detected by checkpatch tool.
> > CHECK: MACRO argument reuse - possible side-effects?
> >
> > Signed-off-by: Jules Irenge
> > ---
> > drivers/staging/rts5208/rtsx_chip.h | 92 +
> > 1 file changed, 55 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/staging/rts5208/rtsx_chip.h
> > b/drivers/staging/rts5208/rtsx_chip.h
> > index bac65784d4a1..4b986d5c68da 100644
> > --- a/drivers/staging/rts5208/rtsx_chip.h
> > +++ b/drivers/staging/rts5208/rtsx_chip.h
> > @@ -386,23 +386,31 @@ struct zone_entry {
> >
> > /* SD card */
> > #define CHK_SD(sd_card)(((sd_card)->sd_type & 0xFF) ==
> > TYPE_SD)
> > -#define CHK_SD_HS(sd_card) (CHK_SD(sd_card) && \
> > -((sd_card)->sd_type & SD_HS))
> > -#define CHK_SD_SDR50(sd_card) (CHK_SD(sd_card) && \
> > -((sd_card)->sd_type & SD_SDR50))
> > -#define CHK_SD_DDR50(sd_card) (CHK_SD(sd_card) && \
> > -((sd_card)->sd_type & SD_DDR50))
> > -#define CHK_SD_SDR104(sd_card) (CHK_SD(sd_card) && \
> > -((sd_card)->sd_type & SD_SDR104))
> > -#define CHK_SD_HCXC(sd_card) (CHK_SD(sd_card) && \
> > -((sd_card)->sd_type & SD_HCXC))
> > -#define CHK_SD_HC(sd_card) (CHK_SD_HCXC(sd_card) && \
> > -((sd_card)->capacity <= 0x400))
> > -#define CHK_SD_XC(sd_card) (CHK_SD_HCXC(sd_card) && \
> > -((sd_card)->capacity > 0x400))
> > -#define CHK_SD30_SPEED(sd_card)(CHK_SD_SDR50(sd_card) || \
> > -CHK_SD_DDR50(sd_card) || \
> > -CHK_SD_SDR104(sd_card))
> > +#define CHK_SD_HS(sd_card)\
> > + ({__auto_type _sd = sd_card; CHK_SD(_sd) && \
> > +(_sd->sd_type & SD_HS); })
> > +#define CHK_SD_SDR50(sd_card)\
> > + ({__auto_type _sd = sd_card; CHK_SD(_sd) && \
> > +(_sd->sd_type & SD_SDR50); })
> > +#define CHK_SD_DDR50(sd_card)\
> > + ({__auto_type _sd = sd_card; CHK_SD(_sd) && \
> > +(_sd->sd_type & SD_DDR50); })
> > +#define CHK_SD_SDR104(sd_card)\
> > + ({__auto_type _sd = sd_card; CHK_SD(_sd) && \
> > +(_sd->sd_type & SD_SDR104); })
> > +#define CHK_SD_HCXC(sd_card)\
> > + ({__auto_type _sd = sd_card; CHK_SD(_sd) && \
> > +(_sd->sd_type & SD_HCXC); })
> > +#define CHK_SD_HC(sd_card)\
> > + ({__auto_type _sd = sd_card; CHK_SD_HCXC(_sd) && \
> > + (_sd->capacity <= 0x400); })
> > +#define CHK_SD_XC(sd_card)\
> > + ({__auto_type _sd = sd_card; CHK_SD_HCXC(_sd) && \
> > +(_sd->capacity > 0x400); })
> > +#define CHK_SD30_SPEED(sd_card)\
> > + ({__auto_type _sd = sd_card; CHK_SD_SDR50(_sd) || \
> > + CHK_SD_DDR50(_sd) || \
> > + CHK_SD_SDR104(_sd); })
> >
> > #define SET_SD(sd_card)((sd_card)->sd_type = TYPE_SD)
> > #define SET_SD_HS(sd_card) ((sd_card)->sd_type |= SD_HS)
> > @@ -420,18 +428,24 @@ struct zone_entry {
> > /* MMC card */
> > #define CHK_MMC(sd_card) (((sd_card)->sd_type & 0xFF) == \
> > TYPE_MMC)
> > -#define CHK_MMC_26M(sd_card) (CHK_MMC(sd_card) && \
> > -((sd_card)->sd_type & MMC_26M))
> > -#define CHK_MMC_52M(sd_card) (CHK_MMC(sd_card) && \
> > -((sd_card)->sd_type & MMC_52M))
> > -#define CHK_MMC_4BIT(sd_card) (CHK_MMC(sd_card) && \
> > -((sd_card)->sd_type & MMC_4BIT))
> > -#define CHK_MMC_8BIT(sd_card) (CHK_MMC(sd_card) && \
> > -((sd_card)->sd_type & MMC_8BIT))
> > -#define CHK_MMC_SECTOR_MODE(sd_card) (CHK_MMC(sd_card) && \
> > -((sd_card)->sd_type & MMC_SECTOR_MODE))
> > -#define CHK_MMC_DDR52(sd_card) (CHK_MMC(sd_card) && \
> > -((sd_card)->sd_type & MMC_DDR52))
> > +#define CHK_MMC_26M(sd_card)\
> > + ({__auto_type _sd = sd_card; CHK_MMC(_sd) && \
> > +(_sd->sd_type & MMC_26M); })
>
> Ick, no. These are obviously pointers, which can not be "evaluated
> twice" so this whole thing is just fine.
>
> checkpatch is just a "hint" that you might want to look at the code.
> This stuff is just fi
Re: [PATCH v1 02/10] KVM: x86/mmu: Prepare kvm_is_mmio_pfn() for PG_reserved changes
On 05.11.19 02:37, Dan Williams wrote:
On Thu, Oct 24, 2019 at 5:10 AM David Hildenbrand wrote:
Right now, ZONE_DEVICE memory is always set PG_reserved. We want to
change that.
KVM has this weird use case that you can map anything from /dev/mem
into the guest. pfn_valid() is not a reliable check whether the memmap
was initialized and can be touched. pfn_to_online_page() makes sure
that we have an initialized memmap (and don't have ZONE_DEVICE memory).
Rewrite kvm_is_mmio_pfn() to make sure the function produces the
same result once we stop setting ZONE_DEVICE pages PG_reserved.
Cc: Paolo Bonzini
Cc: "Radim Krčmář"
Cc: Sean Christopherson
Cc: Vitaly Kuznetsov
Cc: Wanpeng Li
Cc: Jim Mattson
Cc: Joerg Roedel
Cc: Thomas Gleixner
Cc: Ingo Molnar
Cc: Borislav Petkov
Cc: "H. Peter Anvin"
Cc: KarimAllah Ahmed
Cc: Michal Hocko
Cc: Dan Williams
Signed-off-by: David Hildenbrand
---
arch/x86/kvm/mmu.c | 29 +
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 24c23c66b226..f03089a336de 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2962,20 +2962,25 @@ static bool mmu_need_write_protect(struct kvm_vcpu
*vcpu, gfn_t gfn,
static bool kvm_is_mmio_pfn(kvm_pfn_t pfn)
{
+ struct page *page = pfn_to_online_page(pfn);
+
+ /*
+* ZONE_DEVICE pages are never online. Online pages that are reserved
+* either indicate the zero page or MMIO pages.
+*/
+ if (page)
+ return !is_zero_pfn(pfn) && PageReserved(pfn_to_page(pfn));
+
+ /*
+* Anything with a valid (but not online) memmap could be ZONE_DEVICE.
+* Treat only UC/UC-/WC pages as MMIO.
You might clarify that ZONE_DEVICE pages that belong to WB cacheable
pmem have the correct memtype established by devm_memremap_pages().
/*
* Anything with a valid (but not online) memmap could be ZONE_DEVICE.
* Treat only UC/UC-/WC pages as MMIO. devm_memremap_pages() established
* the correct memtype for WB cacheable ZONE_DEVICE pages.
*/
Thanks!
Other than that, feel free to add:
Reviewed-by: Dan Williams
--
Thanks,
David / dhildenb
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: comedi: rewrite macro function with GNU extension typeof
On Mon, 4 Nov 2019, Greg KH wrote: > On Mon, Nov 04, 2019 at 04:33:31PM +, Jules Irenge wrote: > > Rewrite macro function with the GNU extension typeof > > to remove a possible side-effects of MACRO argument reuse "x". > > - Problem could rise if arguments have different types > > and different use though. > > You can not just get away with a potential problem by documenting it :) > > You might have just broken this. Why are you trying to "fix" something > that is not broken? > > What is wrong with the code as-is? > > thanks, > > greg k-h > Again I made a lot of mistake. I will be more carefull. Thanks for the feedback ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: comedi: rewrite macro function with GNU extension typeof
On Mon, 4 Nov 2019, Ian Abbott wrote:
> On 04/11/2019 16:33, Jules Irenge wrote:
> > Rewrite macro function with the GNU extension typeof
> > to remove a possible side-effects of MACRO argument reuse "x".
> > - Problem could rise if arguments have different types
> > and different use though.
> >
> > Signed-off-by: Jules Irenge
> > ---
> > v1 - had no full commit log message, with changes not intended to be in the
> > patch
> > v2 - remove some changes not intended to be in this driver
> > include note of a potential problem
> > drivers/staging/comedi/comedi.h | 6 --
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/comedi/comedi.h
> > b/drivers/staging/comedi/comedi.h
> > index 09a940066c0e..a57691a2e8d8 100644
> > --- a/drivers/staging/comedi/comedi.h
> > +++ b/drivers/staging/comedi/comedi.h
> > @@ -1103,8 +1103,10 @@ enum ni_common_signal_names {
> >
> > /* *** END GLOBALLY-NAMED NI TERMINALS/SIGNALS *** */
> >
> > -#define NI_USUAL_PFI_SELECT(x) (((x) < 10) ? (0x1 + (x)) : (0xb +
> > (x)))
> > -#define NI_USUAL_RTSI_SELECT(x)(((x) < 7) ? (0xb + (x)) : 0x1b)
> > +#define NI_USUAL_PFI_SELECT(x)\
> > + ({typeof(x) x_ = (x); (x_ < 10) ? (0x1 + x_) : (0xb + x_); })
> > +#define NI_USUAL_RTSI_SELECT(x)\
> > + ({typeof(x) x_ = (x); (x_ < 7) ? (0xb + x_) : 0x1b; })
> >
> > /*
> >* mode bits for NI general-purpose counters, set with
> >
>
> I wasn't sure about this the first time around due to the use of GNU
> extensions in uapi header files, but I see there are a few, rare instances of
> this GNU extension elsewhere in other uapi headers (mainly in netfilter
> stuff), so I guess it's OK. However, it does mean that user code that uses
> these macros will no longer compile unless GNU extensions are enabled.
>
> Does anyone know any "best practices" regarding use of GNU extensions in user
> header files under Linux?
>
> --
> -=( Ian Abbott || Web: www.mev.co.uk )=-
> -=( MEV Ltd. is a company registered in England & Wales. )=-
> -=( Registered number: 02862268. Registered address:)=-
> -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
>
>
Apology, I misinterpreted the comments and send a second
version without thinking much.
Please discard it unless you wish to try out.
Any way thanks for the feedback, I really appreciate I find it educative.
Kind regards,
Jules
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 09/10] mm/memory_hotplug: Don't mark pages PG_reserved when initializing the memmap
On 04.11.19 23:44, Boris Ostrovsky wrote:
On 10/24/19 8:09 AM, David Hildenbrand wrote:
diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 4f2e78a5e4db..af69f057913a 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -374,6 +374,13 @@ static void xen_online_page(struct page *page, unsigned
int order)
mutex_lock(&balloon_mutex);
for (i = 0; i < size; i++) {
p = pfn_to_page(start_pfn + i);
+ /*
+* TODO: The core used to mark the pages reserved. Most probably
+* we can stop doing that now. However, especially
+* alloc_xenballooned_pages() left PG_reserved set
+* on pages that can get mapped to user space.
+*/
+ __SetPageReserved(p);
I suspect this is not needed. Pages can get into balloon either from
here or from non-hotplug path (e.g. decrease_reservation()) and so when
we get a page from the balloon we would get a random page that may or
may not have Reserved bit set.
Yeah, I also think it is fine. If you agree, I'll drop this hunk and add
details to the patch description.
--
Thanks,
David / dhildenb
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 03/10] KVM: Prepare kvm_is_reserved_pfn() for PG_reserved changes
On 05.11.19 10:49, David Hildenbrand wrote:
On 05.11.19 10:17, David Hildenbrand wrote:
On 05.11.19 05:38, Dan Williams wrote:
On Thu, Oct 24, 2019 at 5:11 AM David Hildenbrand wrote:
Right now, ZONE_DEVICE memory is always set PG_reserved. We want to
change that.
KVM has this weird use case that you can map anything from /dev/mem
into the guest. pfn_valid() is not a reliable check whether the memmap
was initialized and can be touched. pfn_to_online_page() makes sure
that we have an initialized memmap (and don't have ZONE_DEVICE memory).
Rewrite kvm_is_reserved_pfn() to make sure the function produces the
same result once we stop setting ZONE_DEVICE pages PG_reserved.
Cc: Paolo Bonzini
Cc: "Radim Krčmář"
Cc: Michal Hocko
Cc: Dan Williams
Cc: KarimAllah Ahmed
Signed-off-by: David Hildenbrand
---
virt/kvm/kvm_main.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e9eb666eb6e8..9d18cc67d124 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -151,9 +151,15 @@ __weak int kvm_arch_mmu_notifier_invalidate_range(struct
kvm *kvm,
bool kvm_is_reserved_pfn(kvm_pfn_t pfn)
{
- if (pfn_valid(pfn))
- return PageReserved(pfn_to_page(pfn));
+ struct page *page = pfn_to_online_page(pfn);
+ /*
+* We treat any pages that are not online (not managed by the buddy)
+* as reserved - this includes ZONE_DEVICE pages and pages without
+* a memmap (e.g., mapped via /dev/mem).
+*/
+ if (page)
+ return PageReserved(page);
return true;
}
So after this all the pfn_valid() usage in kvm_main.c is replaced with
pfn_to_online_page()? Looks correct to me.
However, I'm worried that kvm is taking reference on ZONE_DEVICE pages
through some other path resulting in this:
https://lore.kernel.org/linux-nvdimm/20190919154708.ga24...@angband.pl/
I'll see if this patch set modulates or maintains that failure mode.
I'd assume that the behavior is unchanged. Ithink we get a reference to
these ZONE_DEVICE pages via __get_user_pages_fast() and friends in
hva_to_pfn_fast() and friends in virt/kvm/kvm_main.c
I think I know what's going wrong:
Pages that are pinned via gfn_to_pfn() and friends take a references,
however are often released via
kvm_release_pfn_clean()/kvm_release_pfn_dirty()/kvm_release_page_clean()...
E.g., in arch/x86/kvm/x86.c:reexecute_instruction()
...
pfn = gfn_to_pfn(vcpu->kvm, gpa_to_gfn(gpa));
...
kvm_release_pfn_clean(pfn);
void kvm_release_pfn_clean(kvm_pfn_t pfn)
{
if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn))
put_page(pfn_to_page(pfn));
}
This function makes perfect sense as the counterpart for kvm_get_pfn():
void kvm_get_pfn(kvm_pfn_t pfn)
{
if (!kvm_is_reserved_pfn(pfn))
get_page(pfn_to_page(pfn));
}
As all ZONE_DEVICE pages are currently reserved, pages pinned via
gfn_to_pfn() and friends will often not see a put_page() AFAIKS.
Now, my patch does not change that, the result of
kvm_is_reserved_pfn(pfn) will be unchanged. A proper fix for that would
probably be
a) To drop the reference to ZONE_DEVICE pages in gfn_to_pfn() and
friends, after you successfully pinned the pages. (not sure if that's
the right thing to do but you're the expert)
b) To not use kvm_release_pfn_clean() and friends on pages that were
definitely pinned.
(talking to myself, sorry)
Thinking again, dropping this patch from this series could effectively
also fix that issue. E.g., kvm_release_pfn_clean() and friends would
always do a put_page() if "pfn_valid() and !PageReserved()", so after
patch 9 also on ZONDE_DEVICE pages.
But it would have side effects that might not be desired. E.g.,:
1. kvm_pfn_to_page() would also return ZONE_DEVICE pages (might even be
the right thing to do).
2. kvm_set_pfn_dirty() would also set ZONE_DEVICE pages dirty (might be
okay)
--
Thanks,
David / dhildenb
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 4/6] staging: wilc1000: avoid use of C++ style comments
From: Ajay Singh Replace C++ style comment with C style. Signed-off-by: Ajay Singh --- drivers/staging/wilc1000/hif.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/wilc1000/hif.c b/drivers/staging/wilc1000/hif.c index 25f035c02b10..5f6706bcedf6 100644 --- a/drivers/staging/wilc1000/hif.c +++ b/drivers/staging/wilc1000/hif.c @@ -552,7 +552,7 @@ void *wilc_parse_join_bss_param(struct cfg80211_bss *bss, param->mode_802_11i = 2; param->rsn_found = true; - //extract RSN capabilities + /* extract RSN capabilities */ offset += (rsn_ie[offset] * 4) + 2; offset += (rsn_ie[offset] * 4) + 2; memcpy(param->rsn_cap, &rsn_ie[offset], 2); -- 2.22.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 3/6] staging: wilc1000: added 'WILC_' prefix in header guard macro
From: Ajay Singh Use 'WILC_' prefix in header guard to follow the proper naming convention for macro name. Signed-off-by: Ajay Singh --- drivers/staging/wilc1000/cfg80211.h | 4 ++-- drivers/staging/wilc1000/hif.h | 4 ++-- drivers/staging/wilc1000/netdev.h | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/staging/wilc1000/cfg80211.h b/drivers/staging/wilc1000/cfg80211.h index 05c910baf095..5e5d63f70df2 100644 --- a/drivers/staging/wilc1000/cfg80211.h +++ b/drivers/staging/wilc1000/cfg80211.h @@ -4,8 +4,8 @@ * All rights reserved. */ -#ifndef NM_WFI_CFGOPERATIONS -#define NM_WFI_CFGOPERATIONS +#ifndef WILC_CFG80211_H +#define WILC_CFG80211_H #include "netdev.h" struct wiphy *wilc_cfg_alloc(void); diff --git a/drivers/staging/wilc1000/hif.h b/drivers/staging/wilc1000/hif.h index 2defe58ab194..22ee6fffd599 100644 --- a/drivers/staging/wilc1000/hif.h +++ b/drivers/staging/wilc1000/hif.h @@ -4,8 +4,8 @@ * All rights reserved. */ -#ifndef HOST_INT_H -#define HOST_INT_H +#ifndef WILC_HIF_H +#define WILC_HIF_H #include #include "wlan_if.h" diff --git a/drivers/staging/wilc1000/netdev.h b/drivers/staging/wilc1000/netdev.h index 8bc62ce4f2f7..42e0eb192b86 100644 --- a/drivers/staging/wilc1000/netdev.h +++ b/drivers/staging/wilc1000/netdev.h @@ -4,8 +4,8 @@ * All rights reserved. */ -#ifndef WILC_WFI_NETDEVICE -#define WILC_WFI_NETDEVICE +#ifndef WILC_NETDEV_H +#define WILC_NETDEV_H #include #include -- 2.22.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 5/6] staging: wilc1000: added proper spacing for comments
From: Ajay Singh
Added proper space for the comments and added newline before the
comments inside a struct.
Signed-off-by: Ajay Singh
---
drivers/staging/wilc1000/mon.c | 2 +-
drivers/staging/wilc1000/netdev.h | 19 ++-
drivers/staging/wilc1000/wlan.h | 2 +-
drivers/staging/wilc1000/wlan_cfg.c | 2 +-
drivers/staging/wilc1000/wlan_if.h | 8
5 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/wilc1000/mon.c b/drivers/staging/wilc1000/mon.c
index 853fe3056a53..48ac33f06f63 100644
--- a/drivers/staging/wilc1000/mon.c
+++ b/drivers/staging/wilc1000/mon.c
@@ -220,7 +220,7 @@ struct net_device *wilc_wfi_init_mon_interface(struct wilc
*wl,
{
struct wilc_wfi_mon_priv *priv;
- /*If monitor interface is already initialized, return it*/
+ /* If monitor interface is already initialized, return it */
if (wl->monitor_dev)
return wl->monitor_dev;
diff --git a/drivers/staging/wilc1000/netdev.h
b/drivers/staging/wilc1000/netdev.h
index 42e0eb192b86..cd8f0d72caaa 100644
--- a/drivers/staging/wilc1000/netdev.h
+++ b/drivers/staging/wilc1000/netdev.h
@@ -60,7 +60,7 @@ struct sta_info {
u8 sta_associated_bss[WILC_MAX_NUM_STA][ETH_ALEN];
};
-/*Parameters needed for host interface for remaining on channel*/
+/* Parameters needed for host interface for remaining on channel */
struct wilc_wfi_p2p_listen_params {
struct ieee80211_channel *listen_ch;
u32 listen_duration;
@@ -145,11 +145,13 @@ struct wilc_priv {
struct wilc_pmkid_attr pmkid_list;
u8 wep_key[4][WLAN_KEY_LEN_WEP104];
u8 wep_key_len[4];
+
/* The real interface that the monitor is on */
struct net_device *real_ndev;
struct wilc_wfi_key *wilc_gtk[WILC_MAX_NUM_STA];
struct wilc_wfi_key *wilc_ptk[WILC_MAX_NUM_STA];
u8 wilc_groupkey;
+
/* mutexes */
struct mutex scan_req_lock;
bool p2p_listen_state;
@@ -224,16 +226,21 @@ struct wilc {
int close;
u8 vif_num;
struct list_head vif_list;
- /*protect vif list*/
+
+ /* protect vif list */
struct mutex vif_mutex;
struct srcu_struct srcu;
u8 open_ifcs;
- /*protect head of transmit queue*/
+
+ /* protect head of transmit queue */
struct mutex txq_add_to_head_cs;
- /*protect txq_entry_t transmit queue*/
+
+ /* protect txq_entry_t transmit queue */
spinlock_t txq_spinlock;
- /*protect rxq_entry_t receiver queue*/
+
+ /* protect rxq_entry_t receiver queue */
struct mutex rxq_cs;
+
/* lock to protect hif access */
struct mutex hif_cs;
@@ -245,6 +252,7 @@ struct wilc {
struct task_struct *txq_thread;
int quit;
+
/* lock to protect issue of wid command to firmware */
struct mutex cfg_cmd_lock;
struct wilc_cfg_frame cfg_frame;
@@ -271,6 +279,7 @@ struct wilc {
struct wilc_cfg cfg;
void *bus_data;
struct net_device *monitor_dev;
+
/* deinit lock */
struct mutex deinit_lock;
u8 sta_ch;
diff --git a/drivers/staging/wilc1000/wlan.h b/drivers/staging/wilc1000/wlan.h
index 7469fa47d588..1f6957cf2e9c 100644
--- a/drivers/staging/wilc1000/wlan.h
+++ b/drivers/staging/wilc1000/wlan.h
@@ -190,7 +190,7 @@
#define ENABLE_RX_VMM (SEL_VMM_TBL1 | EN_VMM)
#define ENABLE_TX_VMM (SEL_VMM_TBL0 | EN_VMM)
-/*time for expiring the completion of cfg packets*/
+/* time for expiring the completion of cfg packets */
#define WILC_CFG_PKTS_TIMEOUT msecs_to_jiffies(2000)
#define IS_MANAGMEMENT 0x100
diff --git a/drivers/staging/wilc1000/wlan_cfg.c
b/drivers/staging/wilc1000/wlan_cfg.c
index 904f84077ff7..c5b1678c7b5e 100644
--- a/drivers/staging/wilc1000/wlan_cfg.c
+++ b/drivers/staging/wilc1000/wlan_cfg.c
@@ -378,7 +378,7 @@ void wilc_wlan_cfg_indicate_rx(struct wilc *wilc, u8
*frame, int size,
wilc_wlan_parse_info_frame(wilc, frame);
rsp->type = WILC_CFG_RSP_STATUS;
rsp->seq_no = msg_id;
- /*call host interface info parse as well*/
+ /* call host interface info parse as well */
wilc_gnrl_async_info_received(wilc, frame - 4, size + 4);
break;
diff --git a/drivers/staging/wilc1000/wlan_if.h
b/drivers/staging/wilc1000/wlan_if.h
index 70eac586f80c..7c7ee66c35f5 100644
--- a/drivers/staging/wilc1000/wlan_if.h
+++ b/drivers/staging/wilc1000/wlan_if.h
@@ -750,10 +750,10 @@ enum {
WID_REMOVE_KEY = 0x301E,
WID_ASSOC_REQ_INFO = 0x301F,
WID_ASSOC_RES_INFO = 0x3020,
- WID_MANUFACTURER= 0x3026, /*Added for CAPI tool */
- WID_MODEL_NAME = 0x3027, /*Added for CAPI tool */
- WID_MODEL_NUM = 0x3028, /*Added for CAPI tool */
- WID_DEVIC
[PATCH 1/6] staging: wilc1000: avoid 'bool' datatype in struct sent to firmware
From: Ajay Singh
Use 'u8' instead of 'bool' datatype for struct element sent to firmware
because storage of bool datatype is not guaranteed.
Signed-off-by: Ajay Singh
---
drivers/staging/wilc1000/wilc_hif.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/wilc1000/wilc_hif.c
b/drivers/staging/wilc1000/wilc_hif.c
index e0a95c5cc0d5..59eb7600619b 100644
--- a/drivers/staging/wilc1000/wilc_hif.c
+++ b/drivers/staging/wilc1000/wilc_hif.c
@@ -32,7 +32,7 @@ struct wilc_op_mode {
};
struct wilc_reg_frame {
- bool reg;
+ u8 reg;
u8 reg_id;
__le16 frame_type;
} __packed;
@@ -1784,7 +1784,9 @@ void wilc_frame_register(struct wilc_vif *vif, u16
frame_type, bool reg)
wid.val = (u8 *)®_frame;
memset(®_frame, 0x0, sizeof(reg_frame));
- reg_frame.reg = reg;
+
+ if (reg)
+ reg_frame.reg = 1;
switch (frame_type) {
case IEEE80211_STYPE_ACTION:
--
2.22.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 0/6] staging: wilc1000: changes to rename files and few other review comments
From: Ajay Singh
This patch series contains changes to address some of the review
comments received during full driver review [1]. Mainly the changes
are related to files rename and comments formatting.
1. https://www.spinics.net/lists/linux-wireless/msg191489.html
Ajay Singh (6):
staging: wilc1000: avoid 'bool' datatype in struct sent to firmware
staging: wilc1000: remove 'wilc_' prefix from filenames
staging: wilc1000: added 'WILC_' prefix in header guard macro
staging: wilc1000: avoid use of C++ style comments
staging: wilc1000: added proper spacing for comments
staging: wilc1000: use defines for msg types received from firmware
drivers/staging/wilc1000/Makefile | 8 ++---
.../{wilc_wfi_cfgoperations.c => cfg80211.c} | 2 +-
.../{wilc_wfi_cfgoperations.h => cfg80211.h} | 6 ++--
.../staging/wilc1000/{wilc_hif.c => hif.c}| 10 ---
.../staging/wilc1000/{wilc_hif.h => hif.h}| 6 ++--
.../staging/wilc1000/{wilc_mon.c => mon.c}| 4 +--
.../wilc1000/{wilc_netdev.c => netdev.c} | 4 +--
.../{wilc_wfi_netdevice.h => netdev.h}| 29 +++---
.../staging/wilc1000/{wilc_sdio.c => sdio.c} | 4 +--
.../staging/wilc1000/{wilc_spi.c => spi.c}| 4 +--
.../staging/wilc1000/{wilc_wlan.c => wlan.c} | 4 +--
.../staging/wilc1000/{wilc_wlan.h => wlan.h} | 2 +-
.../wilc1000/{wilc_wlan_cfg.c => wlan_cfg.c} | 30 +--
.../wilc1000/{wilc_wlan_cfg.h => wlan_cfg.h} | 0
.../wilc1000/{wilc_wlan_if.h => wlan_if.h}| 8 ++---
15 files changed, 65 insertions(+), 56 deletions(-)
rename drivers/staging/wilc1000/{wilc_wfi_cfgoperations.c => cfg80211.c} (99%)
rename drivers/staging/wilc1000/{wilc_wfi_cfgoperations.h => cfg80211.h} (92%)
rename drivers/staging/wilc1000/{wilc_hif.c => hif.c} (99%)
rename drivers/staging/wilc1000/{wilc_hif.h => hif.h} (99%)
rename drivers/staging/wilc1000/{wilc_mon.c => mon.c} (98%)
rename drivers/staging/wilc1000/{wilc_netdev.c => netdev.c} (99%)
rename drivers/staging/wilc1000/{wilc_wfi_netdevice.h => netdev.h} (95%)
rename drivers/staging/wilc1000/{wilc_sdio.c => sdio.c} (99%)
rename drivers/staging/wilc1000/{wilc_spi.c => spi.c} (99%)
rename drivers/staging/wilc1000/{wilc_wlan.c => wlan.c} (99%)
rename drivers/staging/wilc1000/{wilc_wlan.h => wlan.h} (99%)
rename drivers/staging/wilc1000/{wilc_wlan_cfg.c => wlan_cfg.c} (94%)
rename drivers/staging/wilc1000/{wilc_wlan_cfg.h => wlan_cfg.h} (100%)
rename drivers/staging/wilc1000/{wilc_wlan_if.h => wlan_if.h} (99%)
--
2.22.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 6/6] staging: wilc1000: use defines for msg types received from firmware
From: Ajay Singh
Added defines for different types of messages received from firmware.
Removed the unnecessary comments because after the addition of macros
the message types are self-explanatory.
Signed-off-by: Ajay Singh
---
drivers/staging/wilc1000/wlan_cfg.c | 20 +---
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/wilc1000/wlan_cfg.c
b/drivers/staging/wilc1000/wlan_cfg.c
index c5b1678c7b5e..6f6b286788d1 100644
--- a/drivers/staging/wilc1000/wlan_cfg.c
+++ b/drivers/staging/wilc1000/wlan_cfg.c
@@ -44,6 +44,11 @@ static const struct wilc_cfg_str g_cfg_str[] = {
{WID_NIL, NULL}
};
+#define WILC_RESP_MSG_TYPE_CONFIG_REPLY'R'
+#define WILC_RESP_MSG_TYPE_STATUS_INFO 'I'
+#define WILC_RESP_MSG_TYPE_NETWORK_INFO'N'
+#define WILC_RESP_MSG_TYPE_SCAN_COMPLETE 'S'
+
/
*
* Configuration Functions
@@ -360,21 +365,14 @@ void wilc_wlan_cfg_indicate_rx(struct wilc *wilc, u8
*frame, int size,
size -= 4;
rsp->type = 0;
- /*
-* The valid types of response messages are
-* 'R' (Response),
-* 'I' (Information), and
-* 'N' (Network Information)
-*/
-
switch (msg_type) {
- case 'R':
+ case WILC_RESP_MSG_TYPE_CONFIG_REPLY:
wilc_wlan_parse_response_frame(wilc, frame, size);
rsp->type = WILC_CFG_RSP;
rsp->seq_no = msg_id;
break;
- case 'I':
+ case WILC_RESP_MSG_TYPE_STATUS_INFO:
wilc_wlan_parse_info_frame(wilc, frame);
rsp->type = WILC_CFG_RSP_STATUS;
rsp->seq_no = msg_id;
@@ -382,11 +380,11 @@ void wilc_wlan_cfg_indicate_rx(struct wilc *wilc, u8
*frame, int size,
wilc_gnrl_async_info_received(wilc, frame - 4, size + 4);
break;
- case 'N':
+ case WILC_RESP_MSG_TYPE_NETWORK_INFO:
wilc_network_info_received(wilc, frame - 4, size + 4);
break;
- case 'S':
+ case WILC_RESP_MSG_TYPE_SCAN_COMPLETE:
wilc_scan_complete_received(wilc, frame - 4, size + 4);
break;
--
2.22.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/6] staging: wilc1000: remove 'wilc_' prefix from filenames
From: Ajay Singh
Remove 'wilc_' prefix from filenames, the driver is already present
inside the 'wilc1000' directory so no need to add 'wilc_' in
filenames.
Signed-off-by: Ajay Singh
---
drivers/staging/wilc1000/Makefile | 8
.../wilc1000/{wilc_wfi_cfgoperations.c => cfg80211.c} | 2 +-
.../wilc1000/{wilc_wfi_cfgoperations.h => cfg80211.h} | 2 +-
drivers/staging/wilc1000/{wilc_hif.c => hif.c}| 2 +-
drivers/staging/wilc1000/{wilc_hif.h => hif.h}| 2 +-
drivers/staging/wilc1000/{wilc_mon.c => mon.c}| 2 +-
drivers/staging/wilc1000/{wilc_netdev.c => netdev.c} | 4 ++--
.../staging/wilc1000/{wilc_wfi_netdevice.h => netdev.h} | 6 +++---
drivers/staging/wilc1000/{wilc_sdio.c => sdio.c} | 4 ++--
drivers/staging/wilc1000/{wilc_spi.c => spi.c}| 4 ++--
drivers/staging/wilc1000/{wilc_wlan.c => wlan.c} | 4 ++--
drivers/staging/wilc1000/{wilc_wlan.h => wlan.h} | 0
drivers/staging/wilc1000/{wilc_wlan_cfg.c => wlan_cfg.c} | 8
drivers/staging/wilc1000/{wilc_wlan_cfg.h => wlan_cfg.h} | 0
drivers/staging/wilc1000/{wilc_wlan_if.h => wlan_if.h}| 0
15 files changed, 24 insertions(+), 24 deletions(-)
rename drivers/staging/wilc1000/{wilc_wfi_cfgoperations.c => cfg80211.c} (99%)
rename drivers/staging/wilc1000/{wilc_wfi_cfgoperations.h => cfg80211.h} (97%)
rename drivers/staging/wilc1000/{wilc_hif.c => hif.c} (99%)
rename drivers/staging/wilc1000/{wilc_hif.h => hif.h} (99%)
rename drivers/staging/wilc1000/{wilc_mon.c => mon.c} (99%)
rename drivers/staging/wilc1000/{wilc_netdev.c => netdev.c} (99%)
rename drivers/staging/wilc1000/{wilc_wfi_netdevice.h => netdev.h} (98%)
rename drivers/staging/wilc1000/{wilc_sdio.c => sdio.c} (99%)
rename drivers/staging/wilc1000/{wilc_spi.c => spi.c} (99%)
rename drivers/staging/wilc1000/{wilc_wlan.c => wlan.c} (99%)
rename drivers/staging/wilc1000/{wilc_wlan.h => wlan.h} (100%)
rename drivers/staging/wilc1000/{wilc_wlan_cfg.c => wlan_cfg.c} (98%)
rename drivers/staging/wilc1000/{wilc_wlan_cfg.h => wlan_cfg.h} (100%)
rename drivers/staging/wilc1000/{wilc_wlan_if.h => wlan_if.h} (100%)
diff --git a/drivers/staging/wilc1000/Makefile
b/drivers/staging/wilc1000/Makefile
index a5a8e806b98e..a3305a0a888a 100644
--- a/drivers/staging/wilc1000/Makefile
+++ b/drivers/staging/wilc1000/Makefile
@@ -4,11 +4,11 @@ obj-$(CONFIG_WILC1000) += wilc1000.o
ccflags-y += -DFIRMWARE_1002=\"atmel/wilc1002_firmware.bin\" \
-DFIRMWARE_1003=\"atmel/wilc1003_firmware.bin\"
-wilc1000-objs := wilc_wfi_cfgoperations.o wilc_netdev.o wilc_mon.o \
- wilc_hif.o wilc_wlan_cfg.o wilc_wlan.o
+wilc1000-objs := cfg80211.o netdev.o mon.o \
+ hif.o wlan_cfg.o wlan.o
obj-$(CONFIG_WILC1000_SDIO) += wilc1000-sdio.o
-wilc1000-sdio-objs += wilc_sdio.o
+wilc1000-sdio-objs += sdio.o
obj-$(CONFIG_WILC1000_SPI) += wilc1000-spi.o
-wilc1000-spi-objs += wilc_spi.o
+wilc1000-spi-objs += spi.o
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
b/drivers/staging/wilc1000/cfg80211.c
similarity index 99%
rename from drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
rename to drivers/staging/wilc1000/cfg80211.c
index 66328ac85adc..4863e516ff13 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/cfg80211.c
@@ -4,7 +4,7 @@
* All rights reserved.
*/
-#include "wilc_wfi_cfgoperations.h"
+#include "cfg80211.h"
#define FRAME_TYPE_ID 0
#define ACTION_CAT_ID 24
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.h
b/drivers/staging/wilc1000/cfg80211.h
similarity index 97%
rename from drivers/staging/wilc1000/wilc_wfi_cfgoperations.h
rename to drivers/staging/wilc1000/cfg80211.h
index 7206b6162a8c..05c910baf095 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.h
+++ b/drivers/staging/wilc1000/cfg80211.h
@@ -6,7 +6,7 @@
#ifndef NM_WFI_CFGOPERATIONS
#define NM_WFI_CFGOPERATIONS
-#include "wilc_wfi_netdevice.h"
+#include "netdev.h"
struct wiphy *wilc_cfg_alloc(void);
int wilc_cfg80211_init(struct wilc **wilc, struct device *dev, int io_type,
diff --git a/drivers/staging/wilc1000/wilc_hif.c
b/drivers/staging/wilc1000/hif.c
similarity index 99%
rename from drivers/staging/wilc1000/wilc_hif.c
rename to drivers/staging/wilc1000/hif.c
index 59eb7600619b..25f035c02b10 100644
--- a/drivers/staging/wilc1000/wilc_hif.c
+++ b/drivers/staging/wilc1000/hif.c
@@ -4,7 +4,7 @@
* All rights reserved.
*/
-#include "wilc_wfi_netdevice.h"
+#include "netdev.h"
#define WILC_HIF_SCAN_TIMEOUT_MS5000
#define WILC_HIF_CONNECT_TIMEOUT_MS 9500
diff --git a/drivers/staging/wilc1000/wilc_hif.h
b/drivers/staging/wilc1000/hif.h
similarity index 99%
rename from drivers/staging/wilc1000/wilc_hif.h
rename to drivers/staging/wilc1000/hif.h
index ac5fe57f872b..2defe58ab194 100644
--- a/drivers/staging/w
Re: [PATCH v1 03/10] KVM: Prepare kvm_is_reserved_pfn() for PG_reserved changes
On 05.11.19 10:17, David Hildenbrand wrote:
On 05.11.19 05:38, Dan Williams wrote:
On Thu, Oct 24, 2019 at 5:11 AM David Hildenbrand wrote:
Right now, ZONE_DEVICE memory is always set PG_reserved. We want to
change that.
KVM has this weird use case that you can map anything from /dev/mem
into the guest. pfn_valid() is not a reliable check whether the memmap
was initialized and can be touched. pfn_to_online_page() makes sure
that we have an initialized memmap (and don't have ZONE_DEVICE memory).
Rewrite kvm_is_reserved_pfn() to make sure the function produces the
same result once we stop setting ZONE_DEVICE pages PG_reserved.
Cc: Paolo Bonzini
Cc: "Radim Krčmář"
Cc: Michal Hocko
Cc: Dan Williams
Cc: KarimAllah Ahmed
Signed-off-by: David Hildenbrand
---
virt/kvm/kvm_main.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e9eb666eb6e8..9d18cc67d124 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -151,9 +151,15 @@ __weak int kvm_arch_mmu_notifier_invalidate_range(struct
kvm *kvm,
bool kvm_is_reserved_pfn(kvm_pfn_t pfn)
{
- if (pfn_valid(pfn))
- return PageReserved(pfn_to_page(pfn));
+ struct page *page = pfn_to_online_page(pfn);
+ /*
+* We treat any pages that are not online (not managed by the buddy)
+* as reserved - this includes ZONE_DEVICE pages and pages without
+* a memmap (e.g., mapped via /dev/mem).
+*/
+ if (page)
+ return PageReserved(page);
return true;
}
So after this all the pfn_valid() usage in kvm_main.c is replaced with
pfn_to_online_page()? Looks correct to me.
However, I'm worried that kvm is taking reference on ZONE_DEVICE pages
through some other path resulting in this:
https://lore.kernel.org/linux-nvdimm/20190919154708.ga24...@angband.pl/
I'll see if this patch set modulates or maintains that failure mode.
I'd assume that the behavior is unchanged. Ithink we get a reference to
these ZONE_DEVICE pages via __get_user_pages_fast() and friends in
hva_to_pfn_fast() and friends in virt/kvm/kvm_main.c
I think I know what's going wrong:
Pages that are pinned via gfn_to_pfn() and friends take a references,
however are often released via
kvm_release_pfn_clean()/kvm_release_pfn_dirty()/kvm_release_page_clean()...
E.g., in arch/x86/kvm/x86.c:reexecute_instruction()
...
pfn = gfn_to_pfn(vcpu->kvm, gpa_to_gfn(gpa));
...
kvm_release_pfn_clean(pfn);
void kvm_release_pfn_clean(kvm_pfn_t pfn)
{
if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn))
put_page(pfn_to_page(pfn));
}
This function makes perfect sense as the counterpart for kvm_get_pfn():
void kvm_get_pfn(kvm_pfn_t pfn)
{
if (!kvm_is_reserved_pfn(pfn))
get_page(pfn_to_page(pfn));
}
As all ZONE_DEVICE pages are currently reserved, pages pinned via
gfn_to_pfn() and friends will often not see a put_page() AFAIKS.
Now, my patch does not change that, the result of
kvm_is_reserved_pfn(pfn) will be unchanged. A proper fix for that would
probably be
a) To drop the reference to ZONE_DEVICE pages in gfn_to_pfn() and
friends, after you successfully pinned the pages. (not sure if that's
the right thing to do but you're the expert)
b) To not use kvm_release_pfn_clean() and friends on pages that were
definitely pinned.
--
Thanks,
David / dhildenb
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 01/10] mm/memory_hotplug: Don't allow to online/offline memory blocks with holes
On 05.11.19 02:30, Dan Williams wrote:
On Thu, Oct 24, 2019 at 5:10 AM David Hildenbrand wrote:
Our onlining/offlining code is unnecessarily complicated. Only memory
blocks added during boot can have holes (a range that is not
IORESOURCE_SYSTEM_RAM). Hotplugged memory never has holes (e.g., see
add_memory_resource()). All boot memory is alread online.
s/alread/already/
Thanks.
...also perhaps clarify "already online" by what point in time and why
that is relevant. For example a description of the difference between
the SetPageReserved() in the bootmem path and the one in the hotplug
path.
Will add.
Therefore, when we stop allowing to offline memory blocks with holes, we
implicitly no longer have to deal with onlining memory blocks with holes.
Maybe an explicit reference of the code areas that deal with holes
would help to back up that assertion. Certainly it would have saved me
some time for the review.
I can add a reference to the onlining code that will only online pages
that don't fall into memory holes.
This allows to simplify the code. For example, we no longer have to
worry about marking pages that fall into memory holes PG_reserved when
onlining memory. We can stop setting pages PG_reserved.
...but not for bootmem, right?
Yes, bootmem is not changed. (especially, early allocations and memory
holes are marked PG_reserved - basically everything not given to the
buddy after boot)
Offlining memory blocks added during boot is usually not guranteed to work
s/guranteed/guaranteed/
Thanks.
either way (unmovable data might have easily ended up on that memory during
boot). So stopping to do that should not really hurt (+ people are not
even aware of a setup where that used to work
Maybe put a "Link: https://lkml.kernel.org/r/$msg_id"; to that discussion?
and that the existing code
still works correctly with memory holes). For the use case of offlining
memory to unplug DIMMs, we should see no change. (holes on DIMMs would be
weird).
However, less memory can be offlined than was theoretically allowed
previously, so I don't understand the "we should see no change"
comment. I still agree that's a price worth paying to get the code
cleanups and if someone screams we can look at adding it back, but the
fact that it was already fragile seems decent enough protection.
Hotplugged DIMMs (add_memory()) have no holes and will therefore see no
change.
Please note that hardware errors (PG_hwpoison) are not memory holes and
not affected by this change when offlining.
Cc: Andrew Morton
Cc: Michal Hocko
Cc: Oscar Salvador
Cc: Pavel Tatashin
Cc: Dan Williams
Cc: Anshuman Khandual
Signed-off-by: David Hildenbrand
---
mm/memory_hotplug.c | 26 --
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 561371ead39a..8d81730cf036 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1447,10 +1447,19 @@ static void node_states_clear_node(int node, struct
memory_notify *arg)
node_clear_state(node, N_MEMORY);
}
+static int count_system_ram_pages_cb(unsigned long start_pfn,
+unsigned long nr_pages, void *data)
+{
+ unsigned long *nr_system_ram_pages = data;
+
+ *nr_system_ram_pages += nr_pages;
+ return 0;
+}
+
static int __ref __offline_pages(unsigned long start_pfn,
unsigned long end_pfn)
{
- unsigned long pfn, nr_pages;
+ unsigned long pfn, nr_pages = 0;
unsigned long offlined_pages = 0;
int ret, node, nr_isolate_pageblock;
unsigned long flags;
@@ -1461,6 +1470,20 @@ static int __ref __offline_pages(unsigned long start_pfn,
mem_hotplug_begin();
+ /*
+* Don't allow to offline memory blocks that contain holes.
+* Consecuently, memory blocks with holes can never get onlined
s/Consecuently/Consequently/
Thanks.
+* (hotplugged memory has no holes and all boot memory is online).
+* This allows to simplify the onlining/offlining code quite a lot.
+*/
The last sentence of this comment makes sense in the context of this
patch, but I don't think it stands by itself in the code base after
the fact. The person reading the comment can't see the simplifications
because the code is already gone. I'd clarify it to talk about why it
is safe to not mess around with PG_Reserved in the hotplug path
because of this check.
I'll think of something. It's not just the PG_reserved handling but the
whole pfn_valid()/walk_system_ram_range() handling that can be simplified.
After those clarifications you can add:
Reviewed-by: Dan Williams
Thanks!
--
Thanks,
David / dhildenb
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 03/10] KVM: Prepare kvm_is_reserved_pfn() for PG_reserved changes
On 05.11.19 05:38, Dan Williams wrote:
On Thu, Oct 24, 2019 at 5:11 AM David Hildenbrand wrote:
Right now, ZONE_DEVICE memory is always set PG_reserved. We want to
change that.
KVM has this weird use case that you can map anything from /dev/mem
into the guest. pfn_valid() is not a reliable check whether the memmap
was initialized and can be touched. pfn_to_online_page() makes sure
that we have an initialized memmap (and don't have ZONE_DEVICE memory).
Rewrite kvm_is_reserved_pfn() to make sure the function produces the
same result once we stop setting ZONE_DEVICE pages PG_reserved.
Cc: Paolo Bonzini
Cc: "Radim Krčmář"
Cc: Michal Hocko
Cc: Dan Williams
Cc: KarimAllah Ahmed
Signed-off-by: David Hildenbrand
---
virt/kvm/kvm_main.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e9eb666eb6e8..9d18cc67d124 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -151,9 +151,15 @@ __weak int kvm_arch_mmu_notifier_invalidate_range(struct
kvm *kvm,
bool kvm_is_reserved_pfn(kvm_pfn_t pfn)
{
- if (pfn_valid(pfn))
- return PageReserved(pfn_to_page(pfn));
+ struct page *page = pfn_to_online_page(pfn);
+ /*
+* We treat any pages that are not online (not managed by the buddy)
+* as reserved - this includes ZONE_DEVICE pages and pages without
+* a memmap (e.g., mapped via /dev/mem).
+*/
+ if (page)
+ return PageReserved(page);
return true;
}
So after this all the pfn_valid() usage in kvm_main.c is replaced with
pfn_to_online_page()? Looks correct to me.
However, I'm worried that kvm is taking reference on ZONE_DEVICE pages
through some other path resulting in this:
https://lore.kernel.org/linux-nvdimm/20190919154708.ga24...@angband.pl/
I'll see if this patch set modulates or maintains that failure mode.
I'd assume that the behavior is unchanged. Ithink we get a reference to
these ZONE_DEVICE pages via __get_user_pages_fast() and friends in
hva_to_pfn_fast() and friends in virt/kvm/kvm_main.c
--
Thanks,
David / dhildenb
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/3] media: cedrus: Properly signal size in mode register
Hi,
On Mon 04 Nov 19, 17:33, Jernej Škrabec wrote:
> Dne ponedeljek, 04. november 2019 ob 11:02:28 CET je Paul Kocialkowski
> napisal(a):
> > Hi Jernej,
> >
> > On Sat 26 Oct 19, 09:49, Jernej Skrabec wrote:
> > > Mode register also holds information if video width is bigger than 2048
> > > and if it is equal to 4096.
> > >
> > > Rework cedrus_engine_enable() to properly signal this properties.
> >
> > Thanks for the patch, looks good to me!
> >
> > Acked-by: Paul Kocialkowski
> >
> > One minor thing: maybe we should have a way to set the maximum dimensions
> > depending on the generation of the engine in use and the actual maximum
> > supported by the hardware.
> >
> > Maybe either as dedicated new fields in struct cedrus_variant or as
> > capability flags.
>
> I was thinking about first solution, but after going trough manuals, it was
> unclear what are real limitations. For example, H3 manual states that it is
> capable of decoding H264 1080p@60Hz. However, I know for a fact that it is
> also capable of decoding 4k videos, but probably not at 60 Hz. I don't own
> anything older that A83T, so I don't know what are capabilities of those
> SoCs.
So I guess in this case we should try and see. I could try to look into it at
some point in the future too if you're not particulary interested.
> Anyway, being slow is still ok for some tasks, like transcoding, so we can't
> limit decoding to 1080p just because it's slow. It is probably still faster
> than doing it in SW. Not to mention that it's still ok for some videos, a lot
> of them uses 24 fps.
I agree, it's best to expose the maximum supported resolution by the hardware,
even if it means running at a lower fps.
Do you know if we have a way to report some estimation of the maximum supported
fps to userspace? It would be useful to let userspace decide whether it's a
better fit than software decoding.
Cheers,
Paul
> Best regards,
> Jernej
>
> >
> > Anyway that can be done later since we were already hardcoding this.
> >
> > Cheers,
> >
> > Paul
> >
> > > Signed-off-by: Jernej Skrabec
> > > ---
> > >
> > > drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 2 +-
> > > drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 2 +-
> > > drivers/staging/media/sunxi/cedrus/cedrus_hw.c| 9 +++--
> > > drivers/staging/media/sunxi/cedrus/cedrus_hw.h| 2 +-
> > > drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c | 2 +-
> > > drivers/staging/media/sunxi/cedrus/cedrus_regs.h | 2 ++
> > > 6 files changed, 13 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index
> > > 7487f6ab7576..d2c854ecdf15 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > @@ -485,7 +485,7 @@ static void cedrus_h264_setup(struct cedrus_ctx *ctx,
> > >
> > > {
> > >
> > > struct cedrus_dev *dev = ctx->dev;
> > >
> > > - cedrus_engine_enable(dev, CEDRUS_CODEC_H264);
> > > + cedrus_engine_enable(ctx, CEDRUS_CODEC_H264);
> > >
> > > cedrus_write(dev, VE_H264_SDROT_CTRL, 0);
> > > cedrus_write(dev, VE_H264_EXTRA_BUFFER1,
> > >
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index
> > > 9bc921866f70..6945dc74e1d7 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > @@ -276,7 +276,7 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
> > >
> > > }
> > >
> > > /* Activate H265 engine. */
> > >
> > > - cedrus_engine_enable(dev, CEDRUS_CODEC_H265);
> > > + cedrus_engine_enable(ctx, CEDRUS_CODEC_H265);
> > >
> > > /* Source offset and length in bits. */
> > >
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> > > b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c index
> > > 570a9165dd5d..3acfa21bc124 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> > > @@ -30,7 +30,7 @@
> > >
> > > #include "cedrus_hw.h"
> > > #include "cedrus_regs.h"
> > >
> > > -int cedrus_engine_enable(struct cedrus_dev *dev, enum cedrus_codec codec)
> > > +int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec codec)
> > >
> > > {
> > >
> > > u32 reg = 0;
> > >
> > > @@ -58,7 +58,12 @@ int cedrus_engine_enable(struct cedrus_dev *dev, enum
> > > cedrus_codec codec)>
> > > return -EINVAL;
> > >
> > > }
> > >
> > > - cedrus_write(dev, VE_MODE, reg);
> > > + if (ctx->src_fmt.width == 4096)
> > > + reg |= VE_MODE_PIC_WIDTH_IS_4096;
> > > + if (ctx->src_fmt.width > 2048)
> > > + reg |= VE_MODE_PIC_WIDTH_MORE_2048;
> > > +
> > > + cedrus_write(ctx->dev, VE_MODE, reg);
> > >
> > > return 0;
> > >
> > > }
> > >
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus
Re: [PATCH 2/3] media: cedrus: Fix H264 4k support
Hi Jernej,
On Mon 04 Nov 19, 17:53, Jernej Škrabec wrote:
> Dne ponedeljek, 04. november 2019 ob 11:13:19 CET je Paul Kocialkowski
> napisal(a):
> > Hi,
> >
> > On Sat 26 Oct 19, 09:49, Jernej Skrabec wrote:
> > > H264 decoder needs additional or bigger buffers in order to decode 4k
> > > videos.
> >
> > Thanks for the fixup, we hadn't looked into those bits at all during initial
> > bringup of H.264!
> >
> > See a few minor comments below.
> >
> > > Signed-off-by: Jernej Skrabec
> > > ---
> > >
> > > drivers/staging/media/sunxi/cedrus/cedrus.h | 7 ++
> > > .../staging/media/sunxi/cedrus/cedrus_h264.c | 83 +--
> > > .../staging/media/sunxi/cedrus/cedrus_regs.h | 11 +++
> > > 3 files changed, 93 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > b/drivers/staging/media/sunxi/cedrus/cedrus.h index
> > > c45fb9a7ad07..9676ab8a 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > @@ -116,8 +116,15 @@ struct cedrus_ctx {
> > >
> > > ssize_t mv_col_buf_size;
> > > void*pic_info_buf;
> > > dma_addr_t pic_info_buf_dma;
> > >
> > > + ssize_t pic_info_buf_size;
> > >
> > > void*neighbor_info_buf;
> > > dma_addr_t neighbor_info_buf_dma;
> > >
> > > + void*deblk_buf;
> > > + dma_addr_t deblk_buf_dma;
> > > + ssize_t deblk_buf_size;
> > > + void*intra_pred_buf;
> > > + dma_addr_t intra_pred_buf_dma;
> > > + ssize_t intra_pred_buf_size;
> > >
> > > } h264;
> > > struct {
> > >
> > > void*mv_col_buf;
> > >
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index
> > > d2c854ecdf15..19962f4213d4 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > @@ -39,7 +39,6 @@ struct cedrus_h264_sram_ref_pic {
> > >
> > > #define CEDRUS_H264_FRAME_NUM18
> > >
> > > #define CEDRUS_NEIGHBOR_INFO_BUF_SIZE(16 * SZ_1K)
> > >
> > > -#define CEDRUS_PIC_INFO_BUF_SIZE (128 * SZ_1K)
> >
> > Could we keep a define with the minimum size that you are using later
> > (increased to 130 * SZ_1K)?
>
> Sure.
>
> >
> > > static void cedrus_h264_write_sram(struct cedrus_dev *dev,
> > >
> > > enum cedrus_h264_sram_off off,
> > >
> > > @@ -342,6 +341,20 @@ static void cedrus_set_params(struct cedrus_ctx *ctx,
> > >
> > >VE_H264_VLD_ADDR_FIRST | VE_H264_VLD_ADDR_VALID |
> > >VE_H264_VLD_ADDR_LAST);
> > >
> > > + if (ctx->src_fmt.width > 2048) {
> > > + cedrus_write(dev, VE_BUF_CTRL,
> > > + VE_BUF_CTRL_INTRAPRED_MIXED_RAM |
> > > + VE_BUF_CTRL_DBLK_MIXED_RAM);
> > > + cedrus_write(dev, VE_DBLK_DRAM_BUF_ADDR,
> > > + ctx->codec.h264.deblk_buf_dma);
> > > + cedrus_write(dev, VE_INTRAPRED_DRAM_BUF_ADDR,
> > > + ctx->codec.h264.intra_pred_buf_dma);
> > > + } else {
> > > + cedrus_write(dev, VE_BUF_CTRL,
> > > + VE_BUF_CTRL_INTRAPRED_INT_SRAM |
> > > + VE_BUF_CTRL_DBLK_INT_SRAM);
> > > + }
> > > +
> > >
> > > /*
> > >
> > >* FIXME: Since the bitstream parsing is done in software, and
> > >* in userspace, this shouldn't be needed anymore. But it
> > >
> > > @@ -502,18 +515,28 @@ static void cedrus_h264_setup(struct cedrus_ctx
> > > *ctx,
> > >
> > > static int cedrus_h264_start(struct cedrus_ctx *ctx)
> > > {
> > >
> > > struct cedrus_dev *dev = ctx->dev;
> > >
> > > + unsigned int pic_info_size;
> > >
> > > unsigned int field_size;
> > > unsigned int mv_col_size;
> > > int ret;
> >
> > Maybe add a comment here this is a half-magic sub-optimal formula?
>
> Well, I'm not sure how much suboptimal formulas this and those below are.
> They
> are taken from CedarX source. I would imagine that they didn't waste too much
> memory. What kind of comment would be ok for you? "Formula taken from CedarX
> source"?
Yes, something like that would work fine. The point is to make it clear that
it is not an obvious or direct calculation based on something from the spec.
Cheers,
Paul
> Best regards,
> Jernej
>
> >
> > > + if (ctx->src_fmt.width > 2048)
> > > + pic_info_size = CEDRUS_H264_FRAME_NUM * 0x4000;
> > > + else
> > > + pic_info_size = CEDRUS_H264_FRAME_NUM * 0x1000;
> > > +
> > >
> > > /*
> > >
> > > - * FIXME: It seems that the H6 cedarX code is using a formula
> > > - * here based on
