Re: [PATCH -next 5/5] USB: usbip: Remove an unnecessary NULL value
On 2023/8/5 4:09, shuah wrote:
> On 8/4/23 03:32, Ruan Jinjie wrote:
>> The NULL initialization of the pointers assigned by kzalloc() first is
>> not necessary, because if the kzalloc() failed, the pointers will be
>> assigned NULL, otherwise it works as usual. so remove it.
>>
>> Signed-off-by: Ruan Jinjie
>> ---
>> drivers/usb/usbip/vudc_dev.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
>> index 2bc428f2e261..8e42839e6060 100644
>> --- a/drivers/usb/usbip/vudc_dev.c
>> +++ b/drivers/usb/usbip/vudc_dev.c
>> @@ -489,7 +489,7 @@ static void vudc_device_unusable(struct
>> usbip_device *ud)
>> struct vudc_device *alloc_vudc_device(int devid)
>> {
>> - struct vudc_device *udc_dev = NULL;
>> + struct vudc_device *udc_dev;
>> udc_dev = kzalloc(sizeof(*udc_dev), GFP_KERNEL);
>> if (!udc_dev)
>
> In addition to this, make a change to return NULL instead
> of going to out.
>
> if (!udc_dev)
> goto out;
Thank you! I'll research it and fix sooner.
>
> thanks,
> -- Shuah
>
Re: [PATCH v2 3/3] fork: lock VMAs of the parent process when forking
On Fri, Aug 4, 2023 at 6:06 PM Mateusz Guzik wrote:
>
> On 8/5/23, Linus Torvalds wrote:
> > On Fri, 4 Aug 2023 at 16:25, Mateusz Guzik wrote:
> >>
> >> I know of these guys, I think they are excluded as is -- they go
> >> through access_remote_vm, starting with:
> >> if (mmap_read_lock_killable(mm))
> >> return 0;
> >>
> >> while dup_mmap already write locks the parent's mm.
> >
> > Oh, you're only worried about vma_start_write()?
> >
> > That's a non-issue. It doesn't take the lock normally, since it starts off
> > with
> >
> > if (__is_vma_write_locked(vma, _lock_seq))
> > return;
> >
> > which catches on the lock sequence number already being set.
> >
> > So no extra locking there.
> >
> > Well, technically there's extra locking because the code stupidly
> > doesn't initialize new vma allocations to the right sequence number,
> > but that was talked about here:
> >
> >
> > https://lore.kernel.org/all/CAHk-=wicrwaoeesbuogoqqufvesicbgp3cx0lykgevsfazn...@mail.gmail.com/
> >
> > and it's a separate issue.
> >
>
> I'm going to bet one beer this is the issue.
>
> The patch I'm responding to only consists of adding the call to
> vma_start_write and claims the 5% slowdown from it, while fixing
> crashes if the forking process is multithreaded.
>
> For the fix to work it has to lock something against the parent.
>
> VMA_ITERATOR(old_vmi, oldmm, 0);
> [..]
> for_each_vma(old_vmi, mpnt) {
> [..]
> vma_start_write(mpnt);
>
> the added line locks an obj in the parent's vm space.
>
> The problem you linked looks like pessimization for freshly allocated
> vmas, but that's what is being operated on here.
Sorry, now I'm having trouble understanding the problem you are
describing. We are locking the parent's vma before copying it and the
newly created vma is locked before it's added into the vma tree. What
is the problem then?
>
> --
> Mateusz Guzik
Re: [PATCH v2 3/3] fork: lock VMAs of the parent process when forking
On Fri, Aug 4, 2023 at 6:17 PM Mateusz Guzik wrote: > > On 8/5/23, Suren Baghdasaryan wrote: > > On Fri, Aug 4, 2023 at 5:49 PM Mateusz Guzik wrote: > >> However, the other users (that I know of ) go through the mmap > >> semaphore to mess with anything which means they will wait for > >> dup_mmap to finish (or do their work first). I would be surprised if > >> there were any cases which don't take the semaphore, given that it was > >> a requirement prior to the vma patchset (unless you patched some to no > >> longer need it?). I would guess worst case the semaphore can be added > >> if missing. > > > > No, the only mmap_lock read-lock that is affected is during the page > > fault, which is expected. > > > > I have difficulty parsing your statement. I was just saying that vma lock patchset did not touch any other mmap_locking paths except for the page fault one where we try to skip read-locking mmap_lock. > > I am saying that any 3rd parties which can trigger page faults already > read lock mmap_lock or can be made to do it (and I don't know any case > which does not already, but I'm not willing to spend time poking > around to make sure). One can consider 3rd parties as not a problem, > modulo the audit. > > Past that there does is no known source of trouble? In my original > e-mail I was worried about processes up the chain in ancestry, perhaps > some of the state is shared(?) and the locking at hand neuters any > problems. I'm guessing this is not necessary. > > Bottom line though it looks like this will work fine? > > That said, I'm not going to submit a patch I can't confidently defend. > As I did not dig into any of the VMA code and can't be arsed to audit > all places which mess with "foreign" mm, I'm definitely not submitting > this myself. You are most welcome to write your own variant at your > leisure. :) Ok, I see. I'll need to double check locking when a 3rd party is involved. Will post a patch when I'm confident enough it's safe. Thanks! > > -- > Mateusz Guzik
Re: [PATCH v2 3/3] fork: lock VMAs of the parent process when forking
On 8/5/23, Suren Baghdasaryan wrote: > On Fri, Aug 4, 2023 at 5:49 PM Mateusz Guzik wrote: >> However, the other users (that I know of ) go through the mmap >> semaphore to mess with anything which means they will wait for >> dup_mmap to finish (or do their work first). I would be surprised if >> there were any cases which don't take the semaphore, given that it was >> a requirement prior to the vma patchset (unless you patched some to no >> longer need it?). I would guess worst case the semaphore can be added >> if missing. > > No, the only mmap_lock read-lock that is affected is during the page > fault, which is expected. > I have difficulty parsing your statement. I am saying that any 3rd parties which can trigger page faults already read lock mmap_lock or can be made to do it (and I don't know any case which does not already, but I'm not willing to spend time poking around to make sure). One can consider 3rd parties as not a problem, modulo the audit. Past that there does is no known source of trouble? In my original e-mail I was worried about processes up the chain in ancestry, perhaps some of the state is shared(?) and the locking at hand neuters any problems. I'm guessing this is not necessary. Bottom line though it looks like this will work fine? That said, I'm not going to submit a patch I can't confidently defend. As I did not dig into any of the VMA code and can't be arsed to audit all places which mess with "foreign" mm, I'm definitely not submitting this myself. You are most welcome to write your own variant at your leisure. :) -- Mateusz Guzik
Re: [PATCH v2 3/3] fork: lock VMAs of the parent process when forking
On Fri, Aug 4, 2023 at 5:49 PM Mateusz Guzik wrote: > > On 8/5/23, Suren Baghdasaryan wrote: > > On Fri, Aug 4, 2023 at 5:26 PM Suren Baghdasaryan > > wrote: > >> > >> On Fri, Aug 4, 2023 at 5:15 PM Linus Torvalds > >> wrote: > >> > > >> > On Fri, 4 Aug 2023 at 16:25, Mateusz Guzik wrote: > >> > > > >> > > I know of these guys, I think they are excluded as is -- they go > >> > > through access_remote_vm, starting with: > >> > > if (mmap_read_lock_killable(mm)) > >> > > return 0; > >> > > > >> > > while dup_mmap already write locks the parent's mm. > >> > > >> > Oh, you're only worried about vma_start_write()? > >> > > >> > That's a non-issue. It doesn't take the lock normally, since it starts > >> > off with > >> > > >> > if (__is_vma_write_locked(vma, _lock_seq)) > >> > return; > >> > > >> > which catches on the lock sequence number already being set. > >> > >> That check will prevent re-locking but if vma is not already locked > >> then the call will proceed with obtaining the lock and setting > >> vma->vm_lock_seq to mm->mm_lock_seq. > > > > The optimization Mateusz describes looks valid to me. If there is > > nobody else to fault a page and mm_users is stable (which I think it > > is because we are holding mmap_lock for write) then we can skip vma > > locking, I think. > > > > mm_users is definitely *not* stable -- it can be bumped by > get_task_mm, which is only synchronized with task lock. Ugh, you are of course correct. Poor choice for saying no new users (threads) can appear from under us. > > However, the other users (that I know of ) go through the mmap > semaphore to mess with anything which means they will wait for > dup_mmap to finish (or do their work first). I would be surprised if > there were any cases which don't take the semaphore, given that it was > a requirement prior to the vma patchset (unless you patched some to no > longer need it?). I would guess worst case the semaphore can be added > if missing. No, the only mmap_lock read-lock that is affected is during the page fault, which is expected. > > What is guaranteed is that if the forking process is single-threaded, > there will be no threads added out of nowhere -- the only thread which > could do it is busy creating one in dup_mmap. If multithreaded > operation of the forking process was the only problem, that's it. > > >> > >> > > >> > So no extra locking there. > >> > > >> > Well, technically there's extra locking because the code stupidly > >> > doesn't initialize new vma allocations to the right sequence number, > >> > but that was talked about here: > >> > > >> > > >> > https://lore.kernel.org/all/CAHk-=wicrwaoeesbuogoqqufvesicbgp3cx0lykgevsfazn...@mail.gmail.com/ > >> > > >> > and it's a separate issue. > >> > > >> > Linus > > > > > -- > Mateusz Guzik
Re: [PATCH v2 3/3] fork: lock VMAs of the parent process when forking
On 8/5/23, Linus Torvalds wrote:
> On Fri, 4 Aug 2023 at 16:25, Mateusz Guzik wrote:
>>
>> I know of these guys, I think they are excluded as is -- they go
>> through access_remote_vm, starting with:
>> if (mmap_read_lock_killable(mm))
>> return 0;
>>
>> while dup_mmap already write locks the parent's mm.
>
> Oh, you're only worried about vma_start_write()?
>
> That's a non-issue. It doesn't take the lock normally, since it starts off
> with
>
> if (__is_vma_write_locked(vma, _lock_seq))
> return;
>
> which catches on the lock sequence number already being set.
>
> So no extra locking there.
>
> Well, technically there's extra locking because the code stupidly
> doesn't initialize new vma allocations to the right sequence number,
> but that was talked about here:
>
>
> https://lore.kernel.org/all/CAHk-=wicrwaoeesbuogoqqufvesicbgp3cx0lykgevsfazn...@mail.gmail.com/
>
> and it's a separate issue.
>
I'm going to bet one beer this is the issue.
The patch I'm responding to only consists of adding the call to
vma_start_write and claims the 5% slowdown from it, while fixing
crashes if the forking process is multithreaded.
For the fix to work it has to lock something against the parent.
VMA_ITERATOR(old_vmi, oldmm, 0);
[..]
for_each_vma(old_vmi, mpnt) {
[..]
vma_start_write(mpnt);
the added line locks an obj in the parent's vm space.
The problem you linked looks like pessimization for freshly allocated
vmas, but that's what is being operated on here.
--
Mateusz Guzik
Re: [PATCH v2 3/3] fork: lock VMAs of the parent process when forking
On 8/5/23, Suren Baghdasaryan wrote: > On Fri, Aug 4, 2023 at 5:26 PM Suren Baghdasaryan > wrote: >> >> On Fri, Aug 4, 2023 at 5:15 PM Linus Torvalds >> wrote: >> > >> > On Fri, 4 Aug 2023 at 16:25, Mateusz Guzik wrote: >> > > >> > > I know of these guys, I think they are excluded as is -- they go >> > > through access_remote_vm, starting with: >> > > if (mmap_read_lock_killable(mm)) >> > > return 0; >> > > >> > > while dup_mmap already write locks the parent's mm. >> > >> > Oh, you're only worried about vma_start_write()? >> > >> > That's a non-issue. It doesn't take the lock normally, since it starts >> > off with >> > >> > if (__is_vma_write_locked(vma, _lock_seq)) >> > return; >> > >> > which catches on the lock sequence number already being set. >> >> That check will prevent re-locking but if vma is not already locked >> then the call will proceed with obtaining the lock and setting >> vma->vm_lock_seq to mm->mm_lock_seq. > > The optimization Mateusz describes looks valid to me. If there is > nobody else to fault a page and mm_users is stable (which I think it > is because we are holding mmap_lock for write) then we can skip vma > locking, I think. > mm_users is definitely *not* stable -- it can be bumped by get_task_mm, which is only synchronized with task lock. However, the other users (that I know of ) go through the mmap semaphore to mess with anything which means they will wait for dup_mmap to finish (or do their work first). I would be surprised if there were any cases which don't take the semaphore, given that it was a requirement prior to the vma patchset (unless you patched some to no longer need it?). I would guess worst case the semaphore can be added if missing. What is guaranteed is that if the forking process is single-threaded, there will be no threads added out of nowhere -- the only thread which could do it is busy creating one in dup_mmap. If multithreaded operation of the forking process was the only problem, that's it. >> >> > >> > So no extra locking there. >> > >> > Well, technically there's extra locking because the code stupidly >> > doesn't initialize new vma allocations to the right sequence number, >> > but that was talked about here: >> > >> > >> > https://lore.kernel.org/all/CAHk-=wicrwaoeesbuogoqqufvesicbgp3cx0lykgevsfazn...@mail.gmail.com/ >> > >> > and it's a separate issue. >> > >> > Linus > -- Mateusz Guzik
Re: [PATCH v2 3/3] fork: lock VMAs of the parent process when forking
On Fri, Aug 4, 2023 at 5:26 PM Suren Baghdasaryan wrote: > > On Fri, Aug 4, 2023 at 5:15 PM Linus Torvalds > wrote: > > > > On Fri, 4 Aug 2023 at 16:25, Mateusz Guzik wrote: > > > > > > I know of these guys, I think they are excluded as is -- they go > > > through access_remote_vm, starting with: > > > if (mmap_read_lock_killable(mm)) > > > return 0; > > > > > > while dup_mmap already write locks the parent's mm. > > > > Oh, you're only worried about vma_start_write()? > > > > That's a non-issue. It doesn't take the lock normally, since it starts off > > with > > > > if (__is_vma_write_locked(vma, _lock_seq)) > > return; > > > > which catches on the lock sequence number already being set. > > That check will prevent re-locking but if vma is not already locked > then the call will proceed with obtaining the lock and setting > vma->vm_lock_seq to mm->mm_lock_seq. The optimization Mateusz describes looks valid to me. If there is nobody else to fault a page and mm_users is stable (which I think it is because we are holding mmap_lock for write) then we can skip vma locking, I think. > > > > > So no extra locking there. > > > > Well, technically there's extra locking because the code stupidly > > doesn't initialize new vma allocations to the right sequence number, > > but that was talked about here: > > > > > > https://lore.kernel.org/all/CAHk-=wicrwaoeesbuogoqqufvesicbgp3cx0lykgevsfazn...@mail.gmail.com/ > > > > and it's a separate issue. > > > > Linus
Re: [PATCH v2 3/3] fork: lock VMAs of the parent process when forking
On Fri, Aug 4, 2023 at 5:15 PM Linus Torvalds wrote: > > On Fri, 4 Aug 2023 at 16:25, Mateusz Guzik wrote: > > > > I know of these guys, I think they are excluded as is -- they go > > through access_remote_vm, starting with: > > if (mmap_read_lock_killable(mm)) > > return 0; > > > > while dup_mmap already write locks the parent's mm. > > Oh, you're only worried about vma_start_write()? > > That's a non-issue. It doesn't take the lock normally, since it starts off > with > > if (__is_vma_write_locked(vma, _lock_seq)) > return; > > which catches on the lock sequence number already being set. That check will prevent re-locking but if vma is not already locked then the call will proceed with obtaining the lock and setting vma->vm_lock_seq to mm->mm_lock_seq. > > So no extra locking there. > > Well, technically there's extra locking because the code stupidly > doesn't initialize new vma allocations to the right sequence number, > but that was talked about here: > > > https://lore.kernel.org/all/CAHk-=wicrwaoeesbuogoqqufvesicbgp3cx0lykgevsfazn...@mail.gmail.com/ > > and it's a separate issue. > > Linus
Re: [PATCH v2 3/3] fork: lock VMAs of the parent process when forking
On Fri, 4 Aug 2023 at 16:25, Mateusz Guzik wrote: > > I know of these guys, I think they are excluded as is -- they go > through access_remote_vm, starting with: > if (mmap_read_lock_killable(mm)) > return 0; > > while dup_mmap already write locks the parent's mm. Oh, you're only worried about vma_start_write()? That's a non-issue. It doesn't take the lock normally, since it starts off with if (__is_vma_write_locked(vma, _lock_seq)) return; which catches on the lock sequence number already being set. So no extra locking there. Well, technically there's extra locking because the code stupidly doesn't initialize new vma allocations to the right sequence number, but that was talked about here: https://lore.kernel.org/all/CAHk-=wicrwaoeesbuogoqqufvesicbgp3cx0lykgevsfazn...@mail.gmail.com/ and it's a separate issue. Linus
Re: [PATCH v2 3/3] fork: lock VMAs of the parent process when forking
On 8/5/23, Linus Torvalds wrote: > On Fri, 4 Aug 2023 at 14:46, Mateusz Guzik wrote: >> >> I don't see it mentioned in the discussion, so at a risk of ruffling >> feathers or looking really bad I'm going to ask: is the locking of any >> use if the forking process is single-threaded? T > > Sadly, we've always been able to access the mm from other processes, > so the locking is - I think - unavoidable. > > And some of those "access from other processes" aren't even uncommon > or special. It's things like "ps" etc, that do it just to see the > process name and arguments. > I know of these guys, I think they are excluded as is -- they go through access_remote_vm, starting with: if (mmap_read_lock_killable(mm)) return 0; while dup_mmap already write locks the parent's mm. I don't see any surprise relocks of the semaphore. Granted, should someone *bypass* this mechanism the above would be moot. -- Mateusz Guzik
Re: [PATCH v2 3/3] fork: lock VMAs of the parent process when forking
On Fri, 4 Aug 2023 at 14:46, Mateusz Guzik wrote: > > I don't see it mentioned in the discussion, so at a risk of ruffling > feathers or looking really bad I'm going to ask: is the locking of any > use if the forking process is single-threaded? T Sadly, we've always been able to access the mm from other processes, so the locking is - I think - unavoidable. And some of those "access from other processes" aren't even uncommon or special. It's things like "ps" etc, that do it just to see the process name and arguments. Linus
Re: [PATCH v7 0/8] KVM: allow mapping non-refcounted pages
On Tue, Jul 04, 2023, David Stevens wrote: > From: David Stevens > > This patch series adds support for mapping VM_IO and VM_PFNMAP memory > that is backed by struct pages that aren't currently being refcounted > (e.g. tail pages of non-compound higher order allocations) into the > guest. Aplogies for the slow review, I'm done with feedback for this version. FWIW, it's probably too bit late to catch 6.6, especially since we need acks from ARM and PPC, but 6.7 should be very doable unless someone outright objects. Thanks for being persistent!
Re: [PATCH v7 5/8] KVM: x86/mmu: Don't pass FOLL_GET to __kvm_follow_pfn
On Thu, Jul 06, 2023, David Stevens wrote:
> On Wed, Jul 5, 2023 at 7:17 PM Yu Zhang wrote:
> >
> > On Tue, Jul 04, 2023 at 04:50:50PM +0900, David Stevens wrote:
> > > From: David Stevens
> > >
> > > Stop passing FOLL_GET to __kvm_follow_pfn. This allows the host to map
> > > memory into the guest that is backed by un-refcounted struct pages - for
> > > example, higher order non-compound pages allocated by the amdgpu driver
> > > via ttm_pool_alloc_page.
> >
> > I guess you mean the tail pages of the higher order non-compound pages?
> > And as to the head page, it is said to be set to one coincidentally[*],
> > and shall not be considered as refcounted. IIUC, refcount of this head
> > page will be increased and decreased soon in hva_to_pfn_remapped(), so
> > this may not be a problem(?). But treating this head page differently,
> > as a refcounted one(e.g., to set the A/D flags), is weired.
> >
> > Or maybe I missed some context, e.g., can the head page be allocted to
> > guest at all?
>
> Yes, this is to allow mapping the tail pages of higher order
> non-compound pages - I should have been more precise in my wording.
> The head pages can already be mapped into the guest.
Recording for posterity (or to make an incorrect statment and get corrected),
because I recently had a conversation about the head page not actually being
refcounted. (I can't remember with whom I had the conversation, but I'm pretty
sure it wasn't an imaginary friend).
Even though whatever allocates the page doesn't explicit refcount the head page,
__free_pages() will still do the right thing and (a) keep the head page around
until its last reference is put. And my understanding is that even though it's
a "head" page, it's not a PG_head page, i.e. not a compound page and so is
treated
as an order-0 page when KVM invoke put_page().
void __free_pages(struct page *page, unsigned int order)
{
/* get PageHead before we drop reference */
int head = PageHead(page);
if (put_page_testzero(page)) <=== will evaluate false if KVM holds a
ref
free_the_page(page, order);
else if (!head) <=== will be false for non-compound pages
while (order-- > 0)
free_the_page(page + (1 << order), order);
}
EXPORT_SYMBOL(__free_pages);
Re: [PATCH v7 4/8] KVM: x86/mmu: Migrate to __kvm_follow_pfn
On Wed, Jul 05, 2023, Yu Zhang wrote:
> On Tue, Jul 04, 2023 at 04:50:49PM +0900, David Stevens wrote:
> > From: David Stevens
> >
> > Migrate from __gfn_to_pfn_memslot to __kvm_follow_pfn.
Please turn up your changelog verbosity from ~2 to ~8. E.g. explain the
transition
from async => FOLL_NOWAIT+KVM_PFN_ERR_NEEDS_IO, there's no reason to force
readers
to suss that out on their own.
> > Signed-off-by: David Stevens
> > ---
> > arch/x86/kvm/mmu/mmu.c | 35 +--
> > 1 file changed, 25 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index ec169f5c7dce..e44ab512c3a1 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -4296,7 +4296,12 @@ void kvm_arch_async_page_ready(struct kvm_vcpu
> > *vcpu, struct kvm_async_pf *work)
> > static int __kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault
> > *fault)
> > {
> > struct kvm_memory_slot *slot = fault->slot;
> > - bool async;
> > + struct kvm_follow_pfn foll = {
> > + .slot = slot,
> > + .gfn = fault->gfn,
> > + .flags = FOLL_GET | (fault->write ? FOLL_WRITE : 0),
> > + .allow_write_mapping = true,
> > + };
> >
> > /*
> > * Retry the page fault if the gfn hit a memslot that is being deleted
> > @@ -4325,12 +4330,14 @@ static int __kvm_faultin_pfn(struct kvm_vcpu *vcpu,
> > struct kvm_page_fault *fault
> > return RET_PF_EMULATE;
> > }
> >
> > - async = false;
> > - fault->pfn = __gfn_to_pfn_memslot(slot, fault->gfn, false, false,
> > ,
> > - fault->write, >map_writable,
> > - >hva);
> > - if (!async)
> > - return RET_PF_CONTINUE; /* *pfn has correct page already */
> > + foll.flags |= FOLL_NOWAIT;
> > + fault->pfn = __kvm_follow_pfn();
> > +
> > + if (!is_error_noslot_pfn(fault->pfn))
> > + goto success;
> > +
> > + if (fault->pfn != KVM_PFN_ERR_NEEDS_IO)
> > + return RET_PF_CONTINUE;
>
> IIUC, FOLL_NOWAIT is set only when we wanna an async fault. So
> KVM_PFN_ERR_NEEDS_IO may not be necessary?
But FOLL_NOWAIT is set above. This logic is essentially saying "bail
immediately
if __gfn_to_pfn_memslot() returned a fatal error".
A commented would definitely be helpful though. How about?
/*
* If __kvm_follow_pfn() failed because I/O is needed to fault in the
* page, then either set up an asynchronous #PF to do the I/O, or if
* doing an async #PF isn't possible, retry __kvm_follow_pfn() with
I/O allowed. All other failures are fatal, i.e. retrying won't help.
*/
if (fault->pfn != KVM_PFN_ERR_NEEDS_IO)
return RET_PF_CONTINUE;
Re: [PATCH v7 2/8] KVM: Introduce __kvm_follow_pfn function
On Tue, Jul 04, 2023, David Stevens wrote:
> From: David Stevens
>
> Introduce __kvm_follow_pfn, which will replace __gfn_to_pfn_memslot.
> __kvm_follow_pfn refactors the old API's arguments into a struct and,
> where possible, combines the boolean arguments into a single flags
> argument.
>
> Signed-off-by: David Stevens
> ---
> include/linux/kvm_host.h | 16
> virt/kvm/kvm_main.c | 171 ++-
> virt/kvm/kvm_mm.h| 3 +-
> virt/kvm/pfncache.c | 8 +-
> 4 files changed, 122 insertions(+), 76 deletions(-)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 9d3ac7720da9..ef2763c2b12e 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -97,6 +97,7 @@
> #define KVM_PFN_ERR_HWPOISON (KVM_PFN_ERR_MASK + 1)
> #define KVM_PFN_ERR_RO_FAULT (KVM_PFN_ERR_MASK + 2)
> #define KVM_PFN_ERR_SIGPENDING (KVM_PFN_ERR_MASK + 3)
> +#define KVM_PFN_ERR_NEEDS_IO (KVM_PFN_ERR_MASK + 4)
Hmm, ideally KVM_PFN_ERR_NEEDS_IO would be introduced in a separate prep patch,
e.g. by changing "bool *async" to "bool no_wait". At a glance, I can't tell if
that's feasible though, so consider it more of a "wish" than a request.
> @@ -2572,23 +2561,23 @@ static int kvm_try_get_pfn(kvm_pfn_t pfn)
> return get_page_unless_zero(page);
> }
>
> -static int hva_to_pfn_remapped(struct vm_area_struct *vma,
> -unsigned long addr, bool write_fault,
> -bool *writable, kvm_pfn_t *p_pfn)
> +static int hva_to_pfn_remapped(struct vm_area_struct *vma, struct
> kvm_follow_pfn *foll,
> +kvm_pfn_t *p_pfn)
Please wrap. KVM still honors the 80 char soft limit unless there's a reason
not
to, and in this case it's already wrapping
static int hva_to_pfn_remapped(struct vm_area_struct *vma,
struct kvm_follow_pfn *foll, kvm_pfn_t *p_pfn)
> @@ -2606,8 +2595,7 @@ static int hva_to_pfn_remapped(struct vm_area_struct
> *vma,
> goto out;
> }
>
> - if (writable)
> - *writable = pte_write(*ptep);
> + foll->writable = pte_write(*ptep) && foll->allow_write_mapping;
Similar to feedback in my other response, don't condition this on
try_map_writable,
i.e. just do:
foll->writable = pte_write(...);
> pfn = pte_pfn(*ptep);
>
> /*
> @@ -2652,24 +2640,22 @@ static int hva_to_pfn_remapped(struct vm_area_struct
> *vma,
> * 2): @write_fault = false && @writable, @writable will tell the caller
> * whether the mapping is writable.
> */
> -kvm_pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool interruptible,
> - bool *async, bool write_fault, bool *writable)
> +kvm_pfn_t hva_to_pfn(struct kvm_follow_pfn *foll)
> {
> struct vm_area_struct *vma;
> kvm_pfn_t pfn;
> int npages, r;
>
> /* we can do it either atomically or asynchronously, not both */
> - BUG_ON(atomic && async);
> + BUG_ON(foll->atomic && (foll->flags & FOLL_NOWAIT));
>
> - if (hva_to_pfn_fast(addr, write_fault, writable, ))
> + if (hva_to_pfn_fast(foll, ))
> return pfn;
>
> - if (atomic)
> + if (foll->atomic)
> return KVM_PFN_ERR_FAULT;
>
> - npages = hva_to_pfn_slow(addr, async, write_fault, interruptible,
> - writable, );
> + npages = hva_to_pfn_slow(foll, );
> if (npages == 1)
> return pfn;
> if (npages == -EINTR)
> @@ -2677,83 +2663,122 @@ kvm_pfn_t hva_to_pfn(unsigned long addr, bool
> atomic, bool interruptible,
>
> mmap_read_lock(current->mm);
> if (npages == -EHWPOISON ||
> - (!async && check_user_page_hwpoison(addr))) {
> + (!(foll->flags & FOLL_NOWAIT) &&
> check_user_page_hwpoison(foll->hva))) {
Opportunistically align the indentation, as an added bonus that makes the line
length a few chars shorter, i.e.
if (npages == -EHWPOISON ||
(!(foll->flags & FOLL_NOWAIT) &&
check_user_page_hwpoison(foll->hva))) {
pfn = KVM_PFN_ERR_HWPOISON;
goto exit;
}
Re: [PATCH v7 2/8] KVM: Introduce __kvm_follow_pfn function
On Thu, Jul 06, 2023, Yu Zhang wrote:
> On Thu, Jul 06, 2023 at 02:29:24PM +0900, David Stevens wrote:
> > On Wed, Jul 5, 2023 at 7:53 PM Yu Zhang wrote:
> > >
> > > On Wed, Jul 05, 2023 at 06:22:59PM +0900, David Stevens wrote:
> > > > On Wed, Jul 5, 2023 at 12:10 PM Yu Zhang
> > > > wrote:
> > > > >
> > > > > > @@ -2514,35 +2512,26 @@ static bool hva_to_pfn_fast(unsigned long
> > > > > > addr, bool write_fault,
> > > > > > * The slow path to get the pfn of the specified host virtual
> > > > > > address,
> > > > > > * 1 indicates success, -errno is returned if error is detected.
> > > > > > */
> > > > > > -static int hva_to_pfn_slow(unsigned long addr, bool *async, bool
> > > > > > write_fault,
> > > > > > -bool interruptible, bool *writable,
> > > > > > kvm_pfn_t *pfn)
> > > > > > +static int hva_to_pfn_slow(struct kvm_follow_pfn *foll, kvm_pfn_t
> > > > > > *pfn)
> > > > > > {
> > > > > > - unsigned int flags = FOLL_HWPOISON;
> > > > > > + unsigned int flags = FOLL_HWPOISON | FOLL_GET | foll->flags;
> > > > > > struct page *page;
> > > > > > int npages;
> > > > > >
> > > > > > might_sleep();
> > > > > >
> > > > > > - if (writable)
> > > > > > - *writable = write_fault;
> > > > > > -
> > > > > > - if (write_fault)
> > > > > > - flags |= FOLL_WRITE;
> > > > > > - if (async)
> > > > > > - flags |= FOLL_NOWAIT;
> > > > > > - if (interruptible)
> > > > > > - flags |= FOLL_INTERRUPTIBLE;
> > > > > > -
> > > > > > - npages = get_user_pages_unlocked(addr, 1, , flags);
> > > > > > + npages = get_user_pages_unlocked(foll->hva, 1, , flags);
> > > > > > if (npages != 1)
> > > > > > return npages;
> > > > > >
> > > > > > + foll->writable = (foll->flags & FOLL_WRITE) &&
> > > > > > foll->allow_write_mapping;
> > > > > > +
> > > > > > /* map read fault as writable if possible */
> > > > > > - if (unlikely(!write_fault) && writable) {
> > > > > > + if (unlikely(!foll->writable) && foll->allow_write_mapping) {
> > > > >
> > > > > I guess !foll->writable should be !(foll->flags & FOLL_WRITE) here.
> > > >
> > > > The two statements are logically equivalent, although I guess using
> > > > !(foll->flags & FOLL_WRITE) may be a little clearer, if a little more
> > > > verbose.
> > >
> > > Well, as the comment says, we wanna try to map the read fault as writable
> > > whenever possible. And __gfn_to_pfn_memslot() will only set the FOLL_WRITE
> > > for write faults. So I guess using !foll->writable will not allow this.
> > > Did I miss anything?
> >
> > We just set the foll->writable out parameter to be equal to
> > ((foll->flags & FOLL_WRITE) && foll->allow_write_mapping). Taking a =
> > foll->flags & FOLL_WRITE and b = foll->allow_write_mapping, we have
> > !(a && b) && b -> (!a || !b) && b -> (!a && b) || (!b && b) -> !a &&
> > b.
>
> Ouch, my bad again... I typed "!foll->writable", but missed the "!" in
> my head while calculating... Thanks! :)
The code is funky and confusing though. Specifically, FOLL_WRITE without
allow_write_mapping is nonsensical, and yields the even more nonsensical output
of a successful FOLL_WRITE with foll->writable==%false.
It "works" because callers only consume foll->writable when
foll->allow_write_mapping
is true, but relying on that is ugly and completely unnecessary. Similarly,
the
"allow" terminology is misleading. FOLL_WRITE *always* allows writable
mappings.
This wasn't as much of problem in the previous code because the lower levels
took
the pointer, i.e. avoided the "allow" terminology entirely.
So we should either keep that behavior, i.e. replace "bool allow_write_mapping"
with "bool *writable", or rename allow_write_mapping to something like
opportunistically_map_writable, and then unconditionally set foll->writable
whenever KVM obtains a writable mapping, i.e. regardless of whether the original
fault was a read or a write.
My vote is for the latter. If opportunistically_map_writable is too verbose,
try_map_writable would be another option. Hmm, I'll make "try_map_writable" my
official vote.
Ah, and I also vote to use an if-elif instead of unconditionally setting
foll->writable.
That makes the relationship between FOLL_WRITE and try_map_writable a bit more
obvious IMO. E.g.
static bool hva_to_pfn_fast(struct kvm_follow_pfn *foll, kvm_pfn_t *pfn)
{
struct page *page[1];
/*
* Fast pin a writable pfn only if it is a write fault request
* or the caller allows to map a writable pfn for a read fault
* request.
*/
if (!((foll->flags & FOLL_WRITE) || foll->try_map_writable))
return false;
if (get_user_page_fast_only(foll->hva, FOLL_WRITE, page)) {
*pfn = page_to_pfn(page[0]);
foll->writable = true;
return true;
}
return false;
}
/*
* The slow path
Re: [PATCH v2 3/3] fork: lock VMAs of the parent process when forking
On Sat, Jul 08, 2023 at 12:12:12PM -0700, Suren Baghdasaryan wrote:
[..]
> Lock VMAs of the parent process when forking a child, which prevents
> concurrent page faults during fork operation and avoids this issue.
> This fix can potentially regress some fork-heavy workloads. Kernel build
> time did not show noticeable regression on a 56-core machine while a
> stress test mapping 1 VMAs and forking 5000 times in a tight loop
> shows ~5% regression. If such fork time regression is unacceptable,
> disabling CONFIG_PER_VMA_LOCK should restore its performance. Further
> optimizations are possible if this regression proves to be problematic.
>
> ---
> kernel/fork.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index b85814e614a5..d2e12b6d2b18 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -686,6 +686,7 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
> for_each_vma(old_vmi, mpnt) {
> struct file *file;
>
> + vma_start_write(mpnt);
> if (mpnt->vm_flags & VM_DONTCOPY) {
> vm_stat_account(mm, mpnt->vm_flags, -vma_pages(mpnt));
> continue;
>
I don't see it mentioned in the discussion, so at a risk of ruffling
feathers or looking really bad I'm going to ask: is the locking of any
use if the forking process is single-threaded? The singular thread in
this case is occupied executing this very code, so it can't do any op
in parallel. Is there anyone else who could trigger a page fault? Are
these shared with other processes? Cursory reading suggests a private
copy is made here, so my guess is no. But then again, I landed here
freshly from the interwebz.
Or in short: if nobody can mess up the state if the forking process is
single-threaded, why not check for mm_users or whatever other indicator
to elide the slowdown for the (arguably) most common case?
If the state can be messed up anyway, that's a shame, but short
explanation how would be welcome.
to illustrate (totally untested):
diff --git a/kernel/fork.c b/kernel/fork.c
index d2e12b6d2b18..aac6b08a0b21 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -652,6 +652,7 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
LIST_HEAD(uf);
VMA_ITERATOR(old_vmi, oldmm, 0);
VMA_ITERATOR(vmi, mm, 0);
+ bool singlethread = READ_ONCE(oldmm->mm_users) == 1;
uprobe_start_dup_mmap();
if (mmap_write_lock_killable(oldmm)) {
@@ -686,7 +687,8 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
for_each_vma(old_vmi, mpnt) {
struct file *file;
- vma_start_write(mpnt);
+ if (!singelthreaded)
+ vma_start_write(mpnt);
if (mpnt->vm_flags & VM_DONTCOPY) {
vm_stat_account(mm, mpnt->vm_flags, -vma_pages(mpnt));
continue;
Re: [PATCH -next 5/5] USB: usbip: Remove an unnecessary NULL value
On 8/4/23 03:32, Ruan Jinjie wrote:
The NULL initialization of the pointers assigned by kzalloc() first is
not necessary, because if the kzalloc() failed, the pointers will be
assigned NULL, otherwise it works as usual. so remove it.
Signed-off-by: Ruan Jinjie
---
drivers/usb/usbip/vudc_dev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
index 2bc428f2e261..8e42839e6060 100644
--- a/drivers/usb/usbip/vudc_dev.c
+++ b/drivers/usb/usbip/vudc_dev.c
@@ -489,7 +489,7 @@ static void vudc_device_unusable(struct usbip_device *ud)
struct vudc_device *alloc_vudc_device(int devid)
{
- struct vudc_device *udc_dev = NULL;
+ struct vudc_device *udc_dev;
udc_dev = kzalloc(sizeof(*udc_dev), GFP_KERNEL);
if (!udc_dev)
In addition to this, make a change to return NULL instead
of going to out.
if (!udc_dev)
goto out;
thanks,
-- Shuah
Re: [PATCH] powerpc/mm: Reinstate ARCH_FORCE_MAX_ORDER ranges
Le 19/05/2023 à 13:38, Michael Ellerman a écrit :
> Commit 1e8fed873e74 ("powerpc: drop ranges for definition of
> ARCH_FORCE_MAX_ORDER") removed the limits on the possible values for
> ARCH_FORCE_MAX_ORDER.
>
> However removing the ranges entirely causes some common work flows to
> break. For example building a defconfig (which uses 64K pages), changing
> the page size to 4K, and rebuilding used to work, because
> ARCH_FORCE_MAX_ORDER would be clamped to 12 by the ranges.
>
> With the ranges removed it creates a kernel that builds but crashes at
> boot:
>kernel BUG at mm/huge_memory.c:470!
>Oops: Exception in kernel mode, sig: 5 [#1]
>...
>NIP hugepage_init+0x9c/0x278
>LR do_one_initcall+0x80/0x320
>Call Trace:
> do_one_initcall+0x80/0x320
> kernel_init_freeable+0x304/0x3ac
> kernel_init+0x30/0x1a0
> ret_from_kernel_user_thread+0x14/0x1c
>
> The reasoning for removing the ranges was that some of the values were
> too large. So take that into account and limit the maximums to 10 which
> is the default max, except for the 4K case which uses 12.
There is something wrong:
~# echo 1 > /sys/kernel/mm/hugepages/hugepages-8192kB/nr_hugepages
sh: write error: Invalid argument
$ grep -e MAX_ORDER -e K_PAGES .config
CONFIG_PPC_4K_PAGES=y
# CONFIG_PPC_16K_PAGES is not set
CONFIG_ARCH_FORCE_MAX_ORDER=10
In the past MAX_ORDER used to be 12 but now it is force to 10.
Christophe
>
> Fixes: 1e8fed873e74 ("powerpc: drop ranges for definition of
> ARCH_FORCE_MAX_ORDER")
> Signed-off-by: Michael Ellerman
> ---
> arch/powerpc/Kconfig | 6 ++
> 1 file changed, 6 insertions(+)
>
> I plan to merge this via the powerpc fixes branch.
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 539d1f03ff42..bff5820b7cda 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -906,11 +906,17 @@ config DATA_SHIFT
>
> config ARCH_FORCE_MAX_ORDER
> int "Order of maximal physically contiguous allocations"
> + range 7 8 if PPC64 && PPC_64K_PAGES
> default "8" if PPC64 && PPC_64K_PAGES
> + range 12 12 if PPC64 && !PPC_64K_PAGES
> default "12" if PPC64 && !PPC_64K_PAGES
> + range 8 10 if PPC32 && PPC_16K_PAGES
> default "8" if PPC32 && PPC_16K_PAGES
> + range 6 10 if PPC32 && PPC_64K_PAGES
> default "6" if PPC32 && PPC_64K_PAGES
> + range 4 10 if PPC32 && PPC_256K_PAGES
> default "4" if PPC32 && PPC_256K_PAGES
> + range 10 10
> default "10"
> help
> The kernel page allocator limits the size of maximal physically
Re: [PATCH v3 1/2] nmi_backtrace: Allow excluding an arbitrary CPU
Hi, On Fri, Aug 4, 2023 at 8:02 AM Michal Hocko wrote: > > > > It would have been slightly safer to modify arch_trigger_cpumask_backtrace > > > by switching arguments so that some leftovers are captured easier. > > > > I'm not sure I understand. Oh, you're saying make the prototype of > > arch_trigger_cpumask_backtrace() incompatible so that if someone is > > directly calling it then it'll be a compile-time error? > > exactly. bool to int promotion would be too easy to miss while the > pointer to int would complain loudly. > > > I guess the > > hope is that nobody is calling that directly and they're calling > > through the trigger_...() functions. > > Hope is one thing, being preventive another. > > > For now I'm going to leave this alone. > > If you are going to send another version then please consider this. Not > a hard requirement but better. If I do send another version, do you have any suggestions for how to change this to make it incompatible? I guess swapping the order of the parameters would be best? I considered doing that for v4 but I felt like long term the current order of the parameters was better. I also considered a rename, but that different problems. ;-) If I rename both the #define and the function then if someone has an out-of-tree patch adding arch_trigger_cpumask_backtrace() for another architecture, like say arm64, then there would be no compile-time failure indicating that the out-of-tree patch needs updating. I could rename the functions but _not_ the #define, I guess?
Re: [PATCH] floppy: ERROR: that open brace { should be on the previous line
Hello,
Le 20/07/2023 à 12:17, zhangyongle...@208suo.com a écrit :
> [Vous ne recevez pas souvent de courriers de zhangyongle...@208suo.com.
> D?couvrez pourquoi ceci est important ?
> https://aka.ms/LearnAboutSenderIdentification ]
>
> Fix twoce occurrences of the checkpatch.pl error:
> ERROR: that open brace { should be on the previous line
Can you please explain the purpose of those changes ? Do you use some
tools that get disturbed by such cosmetic errors ? Otherwise what is
your reason ?
We don't accept such standelone minor cosmetic changes at the first
place because it looks like a waste of time.
If you have major reasons to want those changes, please re-submit with a
details explanation in the commit message.
Thanks
Christophe
>
> Signed-off-by: zhangyongle
> ---
> arch/powerpc/include/asm/floppy.h | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/floppy.h
> b/arch/powerpc/include/asm/floppy.h
> index f8ce178b43b7..1ae67d2629be 100644
> --- a/arch/powerpc/include/asm/floppy.h
> +++ b/arch/powerpc/include/asm/floppy.h
> @@ -163,16 +163,14 @@ static int hard_dma_setup(char *addr, unsigned
> long size, int mode, int io)
> return 0;
> }
>
> -static struct fd_dma_ops real_dma_ops =
> -{
> +static struct fd_dma_ops real_dma_ops = {
> ._disable_dma = disable_dma,
> ._free_dma = free_dma,
> ._get_dma_residue = get_dma_residue,
> ._dma_setup = hard_dma_setup
> };
>
> -static struct fd_dma_ops virt_dma_ops =
> -{
> +static struct fd_dma_ops virt_dma_ops = {
> ._disable_dma = vdma_disable_dma,
> ._free_dma = vdma_nop,
> ._get_dma_residue = vdma_get_dma_residue,
> --
> 2.40.1
>
Re: [PATCH] mm: book3s32: add require space around that '?' and ':'
Hello, Le 20/07/2023 à 09:34, hanyu...@208suo.com a écrit : > Fix below checkpatch errors: > > ./arch/powerpc/mm/book3s32/tlb.c:102: ERROR: spaces required around that > '?' (ctx:VxW) > ./arch/powerpc/mm/book3s32/tlb.c:102: ERROR: spaces required around that > ':' (ctx:VxW) Can you please explain the purpose of those changes ? Do you use some tools that get disturbed by such cosmetic errors ? Otherwise what is your reason ? We don't accept such standelone minor cosmetic changes at the first place because it looks like a waste of time. If you have major reasons to want those changes, please re-submit with a details explanation in the commit message. Thanks Christophe > > Signed-off-by: Yu Han > --- > arch/powerpc/mm/book3s32/tlb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/mm/book3s32/tlb.c > b/arch/powerpc/mm/book3s32/tlb.c > index 9ad6b56bfec9..de4abfe3d06b 100644 > --- a/arch/powerpc/mm/book3s32/tlb.c > +++ b/arch/powerpc/mm/book3s32/tlb.c > @@ -99,7 +99,7 @@ void hash__flush_tlb_page(struct vm_area_struct *vma, > unsigned long vmaddr) > struct mm_struct *mm; > pmd_t *pmd; > > - mm = (vmaddr < TASK_SIZE)? vma->vm_mm: _mm; > + mm = (vmaddr < TASK_SIZE) ? vma->vm_mm : _mm; > pmd = pmd_off(mm, vmaddr); > if (!pmd_none(*pmd)) > flush_hash_pages(mm->context.id, vmaddr, pmd_val(*pmd), 1);
Re: [PATCH] powerpc/powernv/pci: use pci_dev_id() to simplify the code
On 04/08/2023 10:04, Xiongfeng Wang wrote:
PCI core API pci_dev_id() can be used to get the BDF number for a pci
device. We don't need to compose it mannually. Use pci_dev_id() to
simplify the code a little bit.
Signed-off-by: Xiongfeng Wang
---
LGTM
Reviewed-by: Frederic Barrat
Fred
arch/powerpc/platforms/powernv/pci-ioda.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c
b/arch/powerpc/platforms/powernv/pci-ioda.c
index cb637827bc58..28fac4770073 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -997,14 +997,14 @@ static void pnv_pci_ioda_dma_dev_setup(struct pci_dev
*pdev)
struct pnv_ioda_pe *pe;
/* Check if the BDFN for this device is associated with a PE yet */
- pe = pnv_pci_bdfn_to_pe(phb, pdev->devfn | (pdev->bus->number << 8));
+ pe = pnv_pci_bdfn_to_pe(phb, pci_dev_id(pdev));
if (!pe) {
/* VF PEs should be pre-configured in pnv_pci_sriov_enable() */
if (WARN_ON(pdev->is_virtfn))
return;
pnv_pci_configure_bus(pdev->bus);
- pe = pnv_pci_bdfn_to_pe(phb, pdev->devfn | (pdev->bus->number
<< 8));
+ pe = pnv_pci_bdfn_to_pe(phb, pci_dev_id(pdev));
pci_info(pdev, "Configured PE#%x\n", pe ? pe->pe_number :
0xf);
@@ -2526,7 +2526,7 @@ static struct iommu_group *pnv_pci_device_group(struct pci_controller *hose,
if (WARN_ON(!phb))
return ERR_PTR(-ENODEV);
- pe = pnv_pci_bdfn_to_pe(phb, pdev->devfn | (pdev->bus->number << 8));
+ pe = pnv_pci_bdfn_to_pe(phb, pci_dev_id(pdev));
if (!pe)
return ERR_PTR(-ENODEV);
Re: [PATCH v3 1/2] nmi_backtrace: Allow excluding an arbitrary CPU
On Fri 04-08-23 06:56:51, Doug Anderson wrote: > Hi, > > On Fri, Aug 4, 2023 at 12:50 AM Michal Hocko wrote: > > > > On Thu 03-08-23 16:07:57, Douglas Anderson wrote: > > > The APIs that allow backtracing across CPUs have always had a way to > > > exclude the current CPU. This convenience means callers didn't need to > > > find a place to allocate a CPU mask just to handle the common case. > > > > > > Let's extend the API to take a CPU ID to exclude instead of just a > > > boolean. This isn't any more complex for the API to handle and allows > > > the hardlockup detector to exclude a different CPU (the one it already > > > did a trace for) without needing to find space for a CPU mask. > > > > > > Arguably, this new API also encourages safer behavior. Specifically if > > > the caller wants to avoid tracing the current CPU (maybe because they > > > already traced the current CPU) this makes it more obvious to the > > > caller that they need to make sure that the current CPU ID can't > > > change. > > > > Yes, this looks like the best way forward. > > > > It would have been slightly safer to modify arch_trigger_cpumask_backtrace > > by switching arguments so that some leftovers are captured easier. > > I'm not sure I understand. Oh, you're saying make the prototype of > arch_trigger_cpumask_backtrace() incompatible so that if someone is > directly calling it then it'll be a compile-time error? exactly. bool to int promotion would be too easy to miss while the pointer to int would complain loudly. > I guess the > hope is that nobody is calling that directly and they're calling > through the trigger_...() functions. Hope is one thing, being preventive another. > For now I'm going to leave this alone. If you are going to send another version then please consider this. Not a hard requirement but better. -- Michal Hocko SUSE Labs
Re: [PATCH v3 1/2] nmi_backtrace: Allow excluding an arbitrary CPU
Hi,
On Fri, Aug 4, 2023 at 12:50 AM Michal Hocko wrote:
>
> On Thu 03-08-23 16:07:57, Douglas Anderson wrote:
> > The APIs that allow backtracing across CPUs have always had a way to
> > exclude the current CPU. This convenience means callers didn't need to
> > find a place to allocate a CPU mask just to handle the common case.
> >
> > Let's extend the API to take a CPU ID to exclude instead of just a
> > boolean. This isn't any more complex for the API to handle and allows
> > the hardlockup detector to exclude a different CPU (the one it already
> > did a trace for) without needing to find space for a CPU mask.
> >
> > Arguably, this new API also encourages safer behavior. Specifically if
> > the caller wants to avoid tracing the current CPU (maybe because they
> > already traced the current CPU) this makes it more obvious to the
> > caller that they need to make sure that the current CPU ID can't
> > change.
>
> Yes, this looks like the best way forward.
>
> It would have been slightly safer to modify arch_trigger_cpumask_backtrace
> by switching arguments so that some leftovers are captured easier.
I'm not sure I understand. Oh, you're saying make the prototype of
arch_trigger_cpumask_backtrace() incompatible so that if someone is
directly calling it then it'll be a compile-time error? I guess the
hope is that nobody is calling that directly and they're calling
through the trigger_...() functions.
For now I'm going to leave this alone.
> You also have this leftover
> diff --git a/include/linux/nmi.h b/include/linux/nmi.h
> index 00982b133dc1..9f1743ee2b28 100644
> --- a/include/linux/nmi.h
> +++ b/include/linux/nmi.h
> @@ -190,10 +190,6 @@ static inline bool trigger_all_cpu_backtrace(void)
> {
> return false;
> }
> -static inline bool trigger_allbutself_cpu_backtrace(void)
> -{
> - return false;
> -}
Ah yes. I missed that case. Let me send a quick v4.
> static inline bool trigger_cpumask_backtrace(struct cpumask *mask)
> {
> return false;
>
> > Signed-off-by: Douglas Anderson
>
> Anyway
> Acked-by: Michal Hocko
Thanks!
[PATCH v4 1/2] nmi_backtrace: Allow excluding an arbitrary CPU
The APIs that allow backtracing across CPUs have always had a way to
exclude the current CPU. This convenience means callers didn't need to
find a place to allocate a CPU mask just to handle the common case.
Let's extend the API to take a CPU ID to exclude instead of just a
boolean. This isn't any more complex for the API to handle and allows
the hardlockup detector to exclude a different CPU (the one it already
did a trace for) without needing to find space for a CPU mask.
Arguably, this new API also encourages safer behavior. Specifically if
the caller wants to avoid tracing the current CPU (maybe because they
already traced the current CPU) this makes it more obvious to the
caller that they need to make sure that the current CPU ID can't
change.
Acked-by: Michal Hocko
Signed-off-by: Douglas Anderson
---
Changes in v4:
- Renamed trigger_allbutself_cpu_backtrace() for when trigger is unsupported.
Changes in v3:
- ("nmi_backtrace: Allow excluding an arbitrary CPU") new for v3.
arch/arm/include/asm/irq.h | 2 +-
arch/arm/kernel/smp.c| 4 ++--
arch/loongarch/include/asm/irq.h | 2 +-
arch/loongarch/kernel/process.c | 4 ++--
arch/mips/include/asm/irq.h | 2 +-
arch/mips/kernel/process.c | 4 ++--
arch/powerpc/include/asm/irq.h | 2 +-
arch/powerpc/kernel/stacktrace.c | 4 ++--
arch/powerpc/kernel/watchdog.c | 4 ++--
arch/sparc/include/asm/irq_64.h | 2 +-
arch/sparc/kernel/process_64.c | 6 +++---
arch/x86/include/asm/irq.h | 2 +-
arch/x86/kernel/apic/hw_nmi.c| 4 ++--
include/linux/nmi.h | 14 +++---
kernel/watchdog.c| 2 +-
lib/nmi_backtrace.c | 6 +++---
16 files changed, 32 insertions(+), 32 deletions(-)
diff --git a/arch/arm/include/asm/irq.h b/arch/arm/include/asm/irq.h
index 18605f1b3580..26c1d2ced4ce 100644
--- a/arch/arm/include/asm/irq.h
+++ b/arch/arm/include/asm/irq.h
@@ -32,7 +32,7 @@ void handle_IRQ(unsigned int, struct pt_regs *);
#include
extern void arch_trigger_cpumask_backtrace(const cpumask_t *mask,
- bool exclude_self);
+ int exclude_cpu);
#define arch_trigger_cpumask_backtrace arch_trigger_cpumask_backtrace
#endif
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 6756203e45f3..3431c0553f45 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -846,7 +846,7 @@ static void raise_nmi(cpumask_t *mask)
__ipi_send_mask(ipi_desc[IPI_CPU_BACKTRACE], mask);
}
-void arch_trigger_cpumask_backtrace(const cpumask_t *mask, bool exclude_self)
+void arch_trigger_cpumask_backtrace(const cpumask_t *mask, int exclude_cpu)
{
- nmi_trigger_cpumask_backtrace(mask, exclude_self, raise_nmi);
+ nmi_trigger_cpumask_backtrace(mask, exclude_cpu, raise_nmi);
}
diff --git a/arch/loongarch/include/asm/irq.h b/arch/loongarch/include/asm/irq.h
index a115e8999c69..218b4da0ea90 100644
--- a/arch/loongarch/include/asm/irq.h
+++ b/arch/loongarch/include/asm/irq.h
@@ -40,7 +40,7 @@ void spurious_interrupt(void);
#define NR_IRQS_LEGACY 16
#define arch_trigger_cpumask_backtrace arch_trigger_cpumask_backtrace
-void arch_trigger_cpumask_backtrace(const struct cpumask *mask, bool
exclude_self);
+void arch_trigger_cpumask_backtrace(const struct cpumask *mask, int
exclude_cpu);
#define MAX_IO_PICS 2
#define NR_IRQS(64 + (256 * MAX_IO_PICS))
diff --git a/arch/loongarch/kernel/process.c b/arch/loongarch/kernel/process.c
index 2e04eb07abb6..778e8d09953e 100644
--- a/arch/loongarch/kernel/process.c
+++ b/arch/loongarch/kernel/process.c
@@ -345,9 +345,9 @@ static void raise_backtrace(cpumask_t *mask)
}
}
-void arch_trigger_cpumask_backtrace(const cpumask_t *mask, bool exclude_self)
+void arch_trigger_cpumask_backtrace(const cpumask_t *mask, int exclude_cpu)
{
- nmi_trigger_cpumask_backtrace(mask, exclude_self, raise_backtrace);
+ nmi_trigger_cpumask_backtrace(mask, exclude_cpu, raise_backtrace);
}
#ifdef CONFIG_64BIT
diff --git a/arch/mips/include/asm/irq.h b/arch/mips/include/asm/irq.h
index 75abfa834ab7..3a848e7e69f7 100644
--- a/arch/mips/include/asm/irq.h
+++ b/arch/mips/include/asm/irq.h
@@ -77,7 +77,7 @@ extern int cp0_fdc_irq;
extern int get_c0_fdc_int(void);
void arch_trigger_cpumask_backtrace(const struct cpumask *mask,
- bool exclude_self);
+ int exclude_cpu);
#define arch_trigger_cpumask_backtrace arch_trigger_cpumask_backtrace
#endif /* _ASM_IRQ_H */
diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index a3225912c862..5387ed0a5186 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -750,9 +750,9 @@ static void raise_backtrace(cpumask_t *mask)
}
}
-void arch_trigger_cpumask_backtrace(const cpumask_t *mask, bool exclude_self)
+void arch_trigger_cpumask_backtrace(const cpumask_t *mask, int
[PATCH net-next v2 09/10] net: fs_enet: Remove linux/fs_enet_pd.h
linux/fs_enet_pd.h is not used anymore.
Remove it.
Signed-off-by: Christophe Leroy
---
MAINTAINERS| 1 -
include/linux/fs_enet_pd.h | 118 -
2 files changed, 119 deletions(-)
delete mode 100644 include/linux/fs_enet_pd.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 5e2bb1059ab6..5bf1be70e4a9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8371,7 +8371,6 @@ L:linuxppc-dev@lists.ozlabs.org
L: net...@vger.kernel.org
S: Maintained
F: drivers/net/ethernet/freescale/fs_enet/
-F: include/linux/fs_enet_pd.h
FREESCALE SOC SOUND DRIVERS
M: Shengjiu Wang
diff --git a/include/linux/fs_enet_pd.h b/include/linux/fs_enet_pd.h
deleted file mode 100644
index 7c9897dab558..
--- a/include/linux/fs_enet_pd.h
+++ /dev/null
@@ -1,118 +0,0 @@
-/*
- * Platform information definitions for the
- * universal Freescale Ethernet driver.
- *
- * Copyright (c) 2003 Intracom S.A.
- * by Pantelis Antoniou
- *
- * 2005 (c) MontaVista Software, Inc.
- * Vitaly Bordug
- *
- * This file is licensed under the terms of the GNU General Public License
- * version 2. This program is licensed "as is" without any warranty of any
- * kind, whether express or implied.
- */
-
-#ifndef FS_ENET_PD_H
-#define FS_ENET_PD_H
-
-#include
-#include
-#include
-#include
-#include
-
-#define FS_ENET_NAME "fs_enet"
-
-enum fs_id {
- fsid_fec1,
- fsid_fec2,
- fsid_fcc1,
- fsid_fcc2,
- fsid_fcc3,
- fsid_scc1,
- fsid_scc2,
- fsid_scc3,
- fsid_scc4,
-};
-
-#define FS_MAX_INDEX 9
-
-static inline int fs_get_fec_index(enum fs_id id)
-{
- if (id >= fsid_fec1 && id <= fsid_fec2)
- return id - fsid_fec1;
- return -1;
-}
-
-static inline int fs_get_fcc_index(enum fs_id id)
-{
- if (id >= fsid_fcc1 && id <= fsid_fcc3)
- return id - fsid_fcc1;
- return -1;
-}
-
-static inline int fs_get_scc_index(enum fs_id id)
-{
- if (id >= fsid_scc1 && id <= fsid_scc4)
- return id - fsid_scc1;
- return -1;
-}
-
-static inline int fs_fec_index2id(int index)
-{
- int id = fsid_fec1 + index - 1;
- if (id >= fsid_fec1 && id <= fsid_fec2)
- return id;
- return FS_MAX_INDEX;
- }
-
-static inline int fs_fcc_index2id(int index)
-{
- int id = fsid_fcc1 + index - 1;
- if (id >= fsid_fcc1 && id <= fsid_fcc3)
- return id;
- return FS_MAX_INDEX;
-}
-
-static inline int fs_scc_index2id(int index)
-{
- int id = fsid_scc1 + index - 1;
- if (id >= fsid_scc1 && id <= fsid_scc4)
- return id;
- return FS_MAX_INDEX;
-}
-
-enum fs_mii_method {
- fsmii_fixed,
- fsmii_fec,
- fsmii_bitbang,
-};
-
-enum fs_ioport {
- fsiop_porta,
- fsiop_portb,
- fsiop_portc,
- fsiop_portd,
- fsiop_porte,
-};
-
-struct fs_mii_bit {
- u32 offset;
- u8 bit;
- u8 polarity;
-};
-struct fs_mii_bb_platform_info {
- struct fs_mii_bit mdio_dir;
- struct fs_mii_bit mdio_dat;
- struct fs_mii_bit mdc_dat;
- int delay; /* delay in us */
- int irq[32];/* irqs per phy's */
-};
-
-struct fs_mii_fec_platform_info {
- u32 irq[32];
- u32 mii_speed;
-};
-
-#endif
--
2.41.0
[PATCH net-next v2 10/10] net: fs_enet: Use cpm_muram_xxx() functions instead of cpm_dpxxx() macros
cpm_dpxxx() macros are now always referring to cpm_muram_xxx() fonctions
directly since commit 3dd82a1ea724 ("[POWERPC] CPM: Always use new
binding.")
Use cpm_muram_xxx() functions directly so that the cpm_dpxxx() macros
can be removed in the near future.
Signed-off-by: Christophe Leroy
---
drivers/net/ethernet/freescale/fs_enet/mac-fcc.c | 2 +-
drivers/net/ethernet/freescale/fs_enet/mac-scc.c | 8
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
b/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
index ce63fd56df89..d903a9012db0 100644
--- a/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
+++ b/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
@@ -105,7 +105,7 @@ static int do_pd_setup(struct fs_enet_private *fep)
goto out_ep;
fep->fcc.mem = (void __iomem *)cpm2_immr;
- fpi->dpram_offset = cpm_dpalloc(128, 32);
+ fpi->dpram_offset = cpm_muram_alloc(128, 32);
if (IS_ERR_VALUE(fpi->dpram_offset)) {
ret = fpi->dpram_offset;
goto out_fcccp;
diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-scc.c
b/drivers/net/ethernet/freescale/fs_enet/mac-scc.c
index 66d40da5cde0..a64cb6270515 100644
--- a/drivers/net/ethernet/freescale/fs_enet/mac-scc.c
+++ b/drivers/net/ethernet/freescale/fs_enet/mac-scc.c
@@ -133,13 +133,13 @@ static int allocate_bd(struct net_device *dev)
struct fs_enet_private *fep = netdev_priv(dev);
const struct fs_platform_info *fpi = fep->fpi;
- fep->ring_mem_addr = cpm_dpalloc((fpi->tx_ring + fpi->rx_ring) *
-sizeof(cbd_t), 8);
+ fep->ring_mem_addr = cpm_muram_alloc((fpi->tx_ring + fpi->rx_ring) *
+sizeof(cbd_t), 8);
if (IS_ERR_VALUE(fep->ring_mem_addr))
return -ENOMEM;
fep->ring_base = (void __iomem __force*)
- cpm_dpram_addr(fep->ring_mem_addr);
+ cpm_muram_addr(fep->ring_mem_addr);
return 0;
}
@@ -149,7 +149,7 @@ static void free_bd(struct net_device *dev)
struct fs_enet_private *fep = netdev_priv(dev);
if (fep->ring_base)
- cpm_dpfree(fep->ring_mem_addr);
+ cpm_muram_free(fep->ring_mem_addr);
}
static void cleanup_data(struct net_device *dev)
--
2.41.0
[PATCH net-next v2 05/10] net: fs_enet: Remove has_phy field in fs_platform_info struct
Since commit 3dd82a1ea724 ("[POWERPC] CPM: Always use new binding.")
has_phy field is never set.
Remove dead code and remove the field.
Signed-off-by: Christophe Leroy
---
drivers/net/ethernet/freescale/fs_enet/mac-fec.c | 14 --
include/linux/fs_enet_pd.h | 1 -
2 files changed, 15 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c
b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c
index f609dc112458..cdc89d83cf07 100644
--- a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c
+++ b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c
@@ -339,11 +339,7 @@ static void restart(struct net_device *dev)
static void stop(struct net_device *dev)
{
struct fs_enet_private *fep = netdev_priv(dev);
- const struct fs_platform_info *fpi = fep->fpi;
struct fec __iomem *fecp = fep->fec.fecp;
-
- struct fec_info *feci = dev->phydev->mdio.bus->priv;
-
int i;
if ((FR(fecp, ecntrl) & FEC_ECNTRL_ETHER_EN) == 0)
@@ -363,16 +359,6 @@ static void stop(struct net_device *dev)
FC(fecp, ecntrl, FEC_ECNTRL_ETHER_EN);
fs_cleanup_bds(dev);
-
- /* shut down FEC1? that's where the mii bus is */
- if (fpi->has_phy) {
- FS(fecp, r_cntrl, fpi->use_rmii ?
- FEC_RCNTRL_RMII_MODE :
- FEC_RCNTRL_MII_MODE); /* MII/RMII enable */
- FS(fecp, ecntrl, FEC_ECNTRL_PINMUX | FEC_ECNTRL_ETHER_EN);
- FW(fecp, ievent, FEC_ENET_MII);
- FW(fecp, mii_speed, feci->mii_speed);
- }
}
static void napi_clear_event_fs(struct net_device *dev)
diff --git a/include/linux/fs_enet_pd.h b/include/linux/fs_enet_pd.h
index a1905e41c167..2b351b676467 100644
--- a/include/linux/fs_enet_pd.h
+++ b/include/linux/fs_enet_pd.h
@@ -123,7 +123,6 @@ struct fs_platform_info {
int napi_weight;/* NAPI weight */
int use_rmii; /* use RMII mode */
- int has_phy;/* if the network is phy container as well...*/
struct clk *clk_per;/* 'per' clock for register access */
};
--
2.41.0
[PATCH net-next v2 08/10] net: fs_enet: Don't include fs_enet_pd.h when not needed
Three platforms in arch/powerpc/platforms/8xx/ include fs_enet_pd.h but don't use anything from it. Remove the includes. Signed-off-by: Christophe Leroy --- arch/powerpc/platforms/8xx/adder875.c| 1 - arch/powerpc/platforms/8xx/mpc885ads_setup.c | 1 - arch/powerpc/platforms/8xx/tqm8xx_setup.c| 1 - 3 files changed, 3 deletions(-) diff --git a/arch/powerpc/platforms/8xx/adder875.c b/arch/powerpc/platforms/8xx/adder875.c index 7e83eb6746f4..f6bd232f8323 100644 --- a/arch/powerpc/platforms/8xx/adder875.c +++ b/arch/powerpc/platforms/8xx/adder875.c @@ -7,7 +7,6 @@ */ #include -#include #include #include diff --git a/arch/powerpc/platforms/8xx/mpc885ads_setup.c b/arch/powerpc/platforms/8xx/mpc885ads_setup.c index 2fc7cacbcd96..c7c4f082b838 100644 --- a/arch/powerpc/platforms/8xx/mpc885ads_setup.c +++ b/arch/powerpc/platforms/8xx/mpc885ads_setup.c @@ -21,7 +21,6 @@ #include #include -#include #include #include #include diff --git a/arch/powerpc/platforms/8xx/tqm8xx_setup.c b/arch/powerpc/platforms/8xx/tqm8xx_setup.c index 7d8eb50bb9cd..6e56be852b2c 100644 --- a/arch/powerpc/platforms/8xx/tqm8xx_setup.c +++ b/arch/powerpc/platforms/8xx/tqm8xx_setup.c @@ -24,7 +24,6 @@ #include #include -#include #include #include #include -- 2.41.0
[PATCH net-next v2 07/10] net: fs_enet: Move struct fs_platform_info into fs_enet.h
struct fs_platform_info is only used in fs_enet ethernet driver since
commit 3dd82a1ea724 ("[POWERPC] CPM: Always use new binding.").
Stale prototypes using fs_platform_info were left over in
arch/powerpc/sysdev/fsl_soc.c but they are now removed by
previous patch.
Move struct fs_platform_info into fs_enet.h
Signed-off-by: Christophe Leroy
---
.../net/ethernet/freescale/fs_enet/fs_enet.h | 19 ++-
.../net/ethernet/freescale/fs_enet/mii-fec.c | 1 +
include/linux/fs_enet_pd.h| 16
3 files changed, 19 insertions(+), 17 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fs_enet/fs_enet.h
b/drivers/net/ethernet/freescale/fs_enet/fs_enet.h
index cb419aef8d1b..d371072fff60 100644
--- a/drivers/net/ethernet/freescale/fs_enet/fs_enet.h
+++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet.h
@@ -2,6 +2,7 @@
#ifndef FS_ENET_H
#define FS_ENET_H
+#include
#include
#include
#include
@@ -9,7 +10,6 @@
#include
#include
-#include
#include
#ifdef CONFIG_CPM1
@@ -118,6 +118,23 @@ struct phy_info {
#define ENET_RX_ALIGN 16
#define ENET_RX_FRSIZE L1_CACHE_ALIGN(PKT_MAXBUF_SIZE + ENET_RX_ALIGN - 1)
+struct fs_platform_info {
+ /* device specific information */
+ u32 cp_command; /* CPM page/sblock/mcn */
+
+ u32 dpram_offset;
+
+ struct device_node *phy_node;
+
+ int rx_ring, tx_ring; /* number of buffers on rx */
+ int rx_copybreak; /* limit we copy small frames */
+ int napi_weight;/* NAPI weight */
+
+ int use_rmii; /* use RMII mode*/
+
+ struct clk *clk_per;/* 'per' clock for register access */
+};
+
struct fs_enet_private {
struct napi_struct napi;
struct device *dev; /* pointer back to the device (must be
initialized first) */
diff --git a/drivers/net/ethernet/freescale/fs_enet/mii-fec.c
b/drivers/net/ethernet/freescale/fs_enet/mii-fec.c
index 1910df250c33..a1e777a4b75f 100644
--- a/drivers/net/ethernet/freescale/fs_enet/mii-fec.c
+++ b/drivers/net/ethernet/freescale/fs_enet/mii-fec.c
@@ -31,6 +31,7 @@
#include
#include
#include
+#include
#include
#include
diff --git a/include/linux/fs_enet_pd.h b/include/linux/fs_enet_pd.h
index 2b351b676467..7c9897dab558 100644
--- a/include/linux/fs_enet_pd.h
+++ b/include/linux/fs_enet_pd.h
@@ -110,22 +110,6 @@ struct fs_mii_bb_platform_info {
int irq[32];/* irqs per phy's */
};
-struct fs_platform_info {
- /* device specific information */
- u32 cp_command; /* CPM page/sblock/mcn */
-
- u32 dpram_offset;
-
- struct device_node *phy_node;
-
- int rx_ring, tx_ring; /* number of buffers on rx */
- int rx_copybreak; /* limit we copy small frames */
- int napi_weight;/* NAPI weight */
-
- int use_rmii; /* use RMII mode */
-
- struct clk *clk_per;/* 'per' clock for register access */
-};
struct fs_mii_fec_platform_info {
u32 irq[32];
u32 mii_speed;
--
2.41.0
[PATCH net-next v2 04/10] net: fs_enet: Remove unused fields in fs_platform_info struct
Since commit 3dd82a1ea724 ("[POWERPC] CPM: Always use new binding.")
many fields of fs_platform_info structure are not used anymore.
Remove them.
Signed-off-by: Christophe Leroy
---
include/linux/fs_enet_pd.h | 19 ---
1 file changed, 19 deletions(-)
diff --git a/include/linux/fs_enet_pd.h b/include/linux/fs_enet_pd.h
index 2351c3d9404d..a1905e41c167 100644
--- a/include/linux/fs_enet_pd.h
+++ b/include/linux/fs_enet_pd.h
@@ -111,33 +111,14 @@ struct fs_mii_bb_platform_info {
};
struct fs_platform_info {
-
- void(*init_ioports)(struct fs_platform_info *);
/* device specific information */
- int fs_no; /* controller index*/
- char fs_type[4];/* controller type */
-
- u32 cp_page;/* CPM page */
- u32 cp_block; /* CPM sblock */
u32 cp_command; /* CPM page/sblock/mcn */
- u32 clk_trx;/* some stuff for pins & mux configuration*/
- u32 clk_rx;
- u32 clk_tx;
- u32 clk_route;
- u32 clk_mask;
-
- u32 mem_offset;
u32 dpram_offset;
- u32 fcc_regs_c;
- u32 device_flags;
-
struct device_node *phy_node;
- const struct fs_mii_bus_info *bus_info;
int rx_ring, tx_ring; /* number of buffers on rx */
- __u8 macaddr[ETH_ALEN]; /* mac address */
int rx_copybreak; /* limit we copy small frames */
int napi_weight;/* NAPI weight */
--
2.41.0
[PATCH net-next v2 03/10] net: fs_enet: Remove fs_get_id()
fs_get_id() hasn't been used since commit b219108cbace ("fs_enet:
Remove !CONFIG_PPC_CPM_NEW_BINDING code")
Remove it.
Signed-off-by: Christophe Leroy
---
include/linux/fs_enet_pd.h | 11 ---
1 file changed, 11 deletions(-)
diff --git a/include/linux/fs_enet_pd.h b/include/linux/fs_enet_pd.h
index 77d783f71527..2351c3d9404d 100644
--- a/include/linux/fs_enet_pd.h
+++ b/include/linux/fs_enet_pd.h
@@ -151,15 +151,4 @@ struct fs_mii_fec_platform_info {
u32 mii_speed;
};
-static inline int fs_get_id(struct fs_platform_info *fpi)
-{
- if(strstr(fpi->fs_type, "SCC"))
- return fs_scc_index2id(fpi->fs_no);
- if(strstr(fpi->fs_type, "FCC"))
- return fs_fcc_index2id(fpi->fs_no);
- if(strstr(fpi->fs_type, "FEC"))
- return fs_fec_index2id(fpi->fs_no);
- return fpi->fs_no;
-}
-
#endif
--
2.41.0
[PATCH net-next v2 06/10] net: fs_enet: Remove stale prototypes from fsl_soc.c
Commit 3dd82a1ea724 ("[POWERPC] CPM: Always use new binding.")
removed last use of init_fec_ioports() and init_fcc_ioports().
Remove stale prototypes then don't include anymore fs_enet_pd.h
which was only included to provide fs_platform_info structure
for the prototypes.
Signed-off-by: Christophe Leroy
---
arch/powerpc/sysdev/fsl_soc.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 68709743450e..c11771542bec 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -23,7 +23,6 @@
#include
#include
#include
-#include
#include
#include
@@ -37,8 +36,6 @@
#include
#include /* For the Freescale hypervisor */
-extern void init_fcc_ioports(struct fs_platform_info*);
-extern void init_fec_ioports(struct fs_platform_info*);
extern void init_smc_ioports(struct fs_uart_platform_info*);
static phys_addr_t immrbase = -1;
--
2.41.0
[PATCH net-next v2 02/10] net: fs_enet: Fix address space and base types mismatches
CHECK drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
drivers/net/ethernet/freescale/fs_enet/mac-fcc.c:550:9: warning: cast removes
address space '__iomem' of expression
drivers/net/ethernet/freescale/fs_enet/mac-fcc.c:550:9: error: subtraction of
different types can't work (different address spaces)
CC drivers/net/ethernet/freescale/fs_enet/mii-bitbang.o
CHECK drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c:95:31: warning: incorrect
type in argument 1 (different base types)
drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c:95:31:expected
unsigned int [noderef] [usertype] __iomem *p
drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c:95:31:got restricted
__be32 [noderef] [usertype] __iomem *dat
...
drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c:63:31: warning: incorrect
type in argument 1 (different base types)
drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c:63:31:expected
unsigned int [noderef] [usertype] __iomem *p
drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c:63:31:got restricted
__be32 [noderef] [usertype] __iomem *dir
...
Fix those address space and base type mismatches reported by sparse.
Signed-off-by: Christophe Leroy
---
drivers/net/ethernet/freescale/fs_enet/mac-fcc.c | 2 +-
drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
b/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
index 925428f1b0c8..ce63fd56df89 100644
--- a/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
+++ b/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
@@ -547,7 +547,7 @@ static void tx_restart(struct net_device *dev)
}
/* Now update the TBPTR and dirty flag to the current buffer */
W32(ep, fen_genfcc.fcc_tbptr,
- (uint) (((void *)recheck_bd - fep->ring_base) +
+ (uint)(((void __iomem *)recheck_bd - fep->ring_base) +
fep->ring_mem_addr));
fep->dirty_tx = recheck_bd;
diff --git a/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
b/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
index 91a69fc2f7c2..f965a2329055 100644
--- a/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
+++ b/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
@@ -29,8 +29,8 @@
struct bb_info {
struct mdiobb_ctrl ctrl;
- __be32 __iomem *dir;
- __be32 __iomem *dat;
+ u32 __iomem *dir;
+ u32 __iomem *dat;
u32 mdio_msk;
u32 mdc_msk;
};
--
2.41.0
[PATCH net-next v2 01/10] net: fs_enet: Remove set but not used variable
CC drivers/net/ethernet/freescale/fs_enet/fs_enet-main.o
drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c: In function
'fs_enet_interrupt':
drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c:321:40: warning: variable
'fpi' set but not used [-Wunused-but-set-variable]
Remove that variable.
Signed-off-by: Christophe Leroy
---
drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
index f9f5b28cc72e..a6dfc8807d3d 100644
--- a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
+++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
@@ -318,14 +318,12 @@ fs_enet_interrupt(int irq, void *dev_id)
{
struct net_device *dev = dev_id;
struct fs_enet_private *fep;
- const struct fs_platform_info *fpi;
u32 int_events;
u32 int_clr_events;
int nr, napi_ok;
int handled;
fep = netdev_priv(dev);
- fpi = fep->fpi;
nr = 0;
while ((int_events = (*fep->ops->get_int_events)(dev)) != 0) {
--
2.41.0
[PATCH net-next v2 00/10] net: fs_enet: Driver cleanup
Over the years, platform and driver initialisation have evolved into more generic ways, and driver or platform specific stuff has gone away, leaving stale objects behind. This series aims at cleaning all that up for fs_enet ethernet driver. Changes in v2: - Remove a trailing whitespace in the old struct moved in patch 7. - Include powerpc people and list that I forgot when sending v1 (and Rob as expected by Patchwork for patch 6, not sure why) Christophe Leroy (10): net: fs_enet: Remove set but not used variable net: fs_enet: Fix address space and base types mismatches net: fs_enet: Remove fs_get_id() net: fs_enet: Remove unused fields in fs_platform_info struct net: fs_enet: Remove has_phy field in fs_platform_info struct net: fs_enet: Remove stale prototypes from fsl_soc.c net: fs_enet: Move struct fs_platform_info into fs_enet.h net: fs_enet: Don't include fs_enet_pd.h when not needed net: fs_enet: Remove linux/fs_enet_pd.h net: fs_enet: Use cpm_muram_xxx() functions instead of cpm_dpxxx() macros MAINTAINERS | 1 - arch/powerpc/platforms/8xx/adder875.c | 1 - arch/powerpc/platforms/8xx/mpc885ads_setup.c | 1 - arch/powerpc/platforms/8xx/tqm8xx_setup.c | 1 - arch/powerpc/sysdev/fsl_soc.c | 3 - .../ethernet/freescale/fs_enet/fs_enet-main.c | 2 - .../net/ethernet/freescale/fs_enet/fs_enet.h | 19 +- .../net/ethernet/freescale/fs_enet/mac-fcc.c | 4 +- .../net/ethernet/freescale/fs_enet/mac-fec.c | 14 -- .../net/ethernet/freescale/fs_enet/mac-scc.c | 8 +- .../ethernet/freescale/fs_enet/mii-bitbang.c | 4 +- .../net/ethernet/freescale/fs_enet/mii-fec.c | 1 + include/linux/fs_enet_pd.h| 165 -- 13 files changed, 27 insertions(+), 197 deletions(-) delete mode 100644 include/linux/fs_enet_pd.h -- 2.41.0
Re: [PATCH] word-at-a-time: use the same return type for has_zero regardless of endianness
On Wed, Aug 02, 2023 at 08:17:32PM +0200, Arnd Bergmann wrote: > On Wed, Aug 2, 2023, at 19:37, Linus Torvalds wrote: > > On Wed, 2 Aug 2023 at 09:16, Nathan Chancellor wrote: > >> > >> We see this warning with ARCH=arm64 defconfig + CONFIG_CPU_BIG_ENDIAN=y. > > > > Oh Christ. I didn't even realize that arm64 allowed a BE config. > > > > The config option goes back to 2013 - are there actually BE user space > > implementations around? > > At least NXP's Layerscape and Huawei's SoCs ended up in big-endian > appliances, running legacy software ported from mips or powerpc. > I agree this was a mistake, but that wasn't nearly as obvious ten > years ago when there were still new BE-only sparc, mips and powerpc > put on the market -- that really only ended in 2017. > > > People, why do we do that? That's positively crazy. BE is dead and > > should be relegated to legacy platforms. There are no advantages to > > being different just for the sake of being different - any "security > > by obscurity" argument would be far outweighed by the inconvenience to > > actual users. > > > > Yes, yes, I know the aarch64 architecture technically allows BE > > implementations - and apparently you can even do it by exception > > level, which I had to look up. But do any actually exist? > > > > Does the kernel even work right in BE mode? It's really easy to miss > > some endianness check when all the actual hardware and use is LE, and > > when (for example) instruction encoding and IO is then always LE > > anyway. > > This was always only done for compatibility with non-portable > software when companies with large custom network stacks argued > that it was cheaper to build the entire open source software to > big-endian than port their own product to little-endian. ;-) > > We (Linaro) used to test all toolchain and kernel releases in > big-endian mode as member companies had customers that asked > for it, but that stopped a while ago as those legacy software > stacks either got more portable or got replaced over time. > > Many Arm systems won't boot BE kernels any more because UEFI > firmware only supports LE, or because of driver bugs. > Virtual machines are still likely to work fine though. > I'm fairly sure that all Arm Cortex and Neoverse cores still\ > support BE mode in all exception levels, OTOH at least Apple's > custom CPUs do not implement it at all. Yes, that's right. The CPUs we have *do* tend to support BE, but practically there isn't any software to run on them. I asked about removing it a few years ago: https://lore.kernel.org/linux-arm-kernel/20191011102747.lpbaur2e4nqyf7sw@willie-the-truck/ but Hanjun said that Huawei are using it, so it stayed. Will
Re: [RFC PATCH v2 0/7] Add audio support in v4l2 framework
On 04/08/2023 14:19, Shengjiu Wang wrote: > On Wed, Aug 2, 2023 at 8:28 PM Hans Verkuil wrote: >> >> On 02/08/2023 14:02, Shengjiu Wang wrote: >>> On Wed, Aug 2, 2023 at 7:22 PM Takashi Iwai wrote: On Wed, 02 Aug 2023 09:32:37 +0200, Hans Verkuil wrote: > > Hi all, > > On 25/07/2023 08:12, Shengjiu Wang wrote: >> Audio signal processing has the requirement for memory to >> memory similar as Video. >> >> This patch is to add this support in v4l2 framework, defined >> new buffer type V4L2_BUF_TYPE_AUDIO_CAPTURE and >> V4L2_BUF_TYPE_AUDIO_OUTPUT, defined new format v4l2_audio_format >> for audio case usage. >> >> The created audio device is named "/dev/audioX". >> >> And add memory to memory support for two kinds of i.MX ASRC >> module > > Before I spend time on this: are the audio maintainers OK with doing > this in V4L2? > > I do want to have a clear statement on this as it is not something I > can decide. Well, I personally don't mind to have some audio capability in v4l2 layer. But, the only uncertain thing for now is whether this is a must-have or not. >>> >>> Thanks, I am also not sure about this. I am also confused that why >>> there is no m2m implementation for audio in the kernel. Audio also >>> has similar decoder encoder post-processing as video. >>> IIRC, the implementation in the sound driver side was never done just because there was no similar implementation? If so, and if the extension to the v4l2 core layer is needed, shouldn't it be more considered for the possible other route? >>> >>> Actually I'd like someone could point me to the other route. I'd like to >>> try. >>> >>> The reason why I select to extend v4l2 for such audio usage is that v4l2 >>> looks best for this audio m2m implementation. v4l2 is designed for m2m >>> usage. if we need implement another 'route', I don't think it can do >>> better >>> that v4l2. >>> >>> I appreciate that someone can share his ideas or doable solutions. >>> And please don't ignore my request, ignore my patch. >> >> To give a bit more background: if it is decided to use the v4l API for this >> (and I have no objection to this from my side since API/framework-wise it is >> a >> good fit for this), then there are a number of things that need to be done to >> get this into the media subsystem: >> >> - documentation for the new uAPI >> - add support for this to v4l2-ctl >> - add v4l2-compliance tests for the new device >> - highly desirable: have a virtual driver (similar to vim2m) that supports >> this: >> it could be as simple as just copy input to output. This helps regression >> testing. >> - it might need media controller support as well. TBD. >> >> None of this is particularly complex, but taken all together it is a fair >> amount of work that also needs a lot of review time from our side. >> >> I want to add one more option to the mix: drivers/media/core/v4l2-mem2mem.c >> is >> the main m2m framework, but it relies heavily on the videobuf2 framework for >> the capture and output queues. >> >> The core vb2 implementation in >> drivers/media/common/videobuf2/videobuf2-core.c >> is independent of V4L2 and can be used by other subsystems (in our case, it >> is >> also used by the DVB API). It is a possibility to create an alsa version of >> v4l2-mem2mem.c that uses the core vb2 code with an ALSA uAPI on top. >> >> So in drivers/media/common/videobuf2/ you would have a videobuf2-alsa.c >> besides >> the already existing videobuf2-v4l2.c and -dvb.c. >> >> Perhaps parts of v4l2-mem2mem.c can be reused as well in that case, but I am >> not sure if it is worth the effort. I suspect copying it to an alsa-mem2mem.c >> and adapting it for alsa is easiest if you want to go that way. >> > > Thanks. > > Does this means that videobuf2-v4l2.c and v4l2-mem2mem.c are dedicate > for video device? if audio want to use v4l2 framework, need to create > videobuf2-alsa.c and alsa-mem2mem.c, but it may cause a lot of function > duplicate. The videobuf2-v4l2.c sits on top of videobuf2-core.c and provides the V4L2 uAPI for the streaming functionality. If you don't want to use the V4L2 uAPI for this, then you would need a videobuf2-alsa.c that provides a (possibly new) ALSA uAPI. Whether that makes sense is something I cannot decide. v4l2-mem2mem.c uses videobuf2-v4l2.c, so if you need a ALSA version, then you probably need to create an alsa-mem2mem.c (possibly some functionality can be shared). It's just a third option, and it can be useful if there is a strong desire to keep the uAPI for this functionality entirely within the ALSA subsystem, but you want to reuse the streaming I/O functionality that the videobuf2 core provides. If the decision is that it is fine to use the V4L2 uAPI for this type of audio functionality through a /dev/v4l-audioX device, then just ignore this option and use V4L2.
Re: [RFC PATCH v2 0/7] Add audio support in v4l2 framework
On Wed, Aug 2, 2023 at 8:28 PM Hans Verkuil wrote: > > On 02/08/2023 14:02, Shengjiu Wang wrote: > > On Wed, Aug 2, 2023 at 7:22 PM Takashi Iwai wrote: > >> > >> On Wed, 02 Aug 2023 09:32:37 +0200, > >> Hans Verkuil wrote: > >>> > >>> Hi all, > >>> > >>> On 25/07/2023 08:12, Shengjiu Wang wrote: > Audio signal processing has the requirement for memory to > memory similar as Video. > > This patch is to add this support in v4l2 framework, defined > new buffer type V4L2_BUF_TYPE_AUDIO_CAPTURE and > V4L2_BUF_TYPE_AUDIO_OUTPUT, defined new format v4l2_audio_format > for audio case usage. > > The created audio device is named "/dev/audioX". > > And add memory to memory support for two kinds of i.MX ASRC > module > >>> > >>> Before I spend time on this: are the audio maintainers OK with doing > >>> this in V4L2? > >>> > >>> I do want to have a clear statement on this as it is not something I > >>> can decide. > >> > >> Well, I personally don't mind to have some audio capability in v4l2 > >> layer. But, the only uncertain thing for now is whether this is a > >> must-have or not. > >> > > > > Thanks, I am also not sure about this. I am also confused that why > > there is no m2m implementation for audio in the kernel. Audio also > > has similar decoder encoder post-processing as video. > > > >> > >> IIRC, the implementation in the sound driver side was never done just > >> because there was no similar implementation? If so, and if the > >> extension to the v4l2 core layer is needed, shouldn't it be more > >> considered for the possible other route? > >> > > > > Actually I'd like someone could point me to the other route. I'd like to > > try. > > > > The reason why I select to extend v4l2 for such audio usage is that v4l2 > > looks best for this audio m2m implementation. v4l2 is designed for m2m > > usage. if we need implement another 'route', I don't think it can do > > better > > that v4l2. > > > > I appreciate that someone can share his ideas or doable solutions. > > And please don't ignore my request, ignore my patch. > > To give a bit more background: if it is decided to use the v4l API for this > (and I have no objection to this from my side since API/framework-wise it is a > good fit for this), then there are a number of things that need to be done to > get this into the media subsystem: > > - documentation for the new uAPI > - add support for this to v4l2-ctl > - add v4l2-compliance tests for the new device > - highly desirable: have a virtual driver (similar to vim2m) that supports > this: > it could be as simple as just copy input to output. This helps regression > testing. > - it might need media controller support as well. TBD. > > None of this is particularly complex, but taken all together it is a fair > amount of work that also needs a lot of review time from our side. > > I want to add one more option to the mix: drivers/media/core/v4l2-mem2mem.c is > the main m2m framework, but it relies heavily on the videobuf2 framework for > the capture and output queues. > > The core vb2 implementation in drivers/media/common/videobuf2/videobuf2-core.c > is independent of V4L2 and can be used by other subsystems (in our case, it is > also used by the DVB API). It is a possibility to create an alsa version of > v4l2-mem2mem.c that uses the core vb2 code with an ALSA uAPI on top. > > So in drivers/media/common/videobuf2/ you would have a videobuf2-alsa.c > besides > the already existing videobuf2-v4l2.c and -dvb.c. > > Perhaps parts of v4l2-mem2mem.c can be reused as well in that case, but I am > not sure if it is worth the effort. I suspect copying it to an alsa-mem2mem.c > and adapting it for alsa is easiest if you want to go that way. > Thanks. Does this means that videobuf2-v4l2.c and v4l2-mem2mem.c are dedicate for video device? if audio want to use v4l2 framework, need to create videobuf2-alsa.c and alsa-mem2mem.c, but it may cause a lot of function duplicate. Best regards Wang Shengjiu
[PATCH -next] soc: fsl: dpio: Remove redundant initialization owner in dpaa2_dpio_driver
The fsl_mc_driver_register() will set "THIS_MODULE" to driver.owner when
register a fsl_mc_driver driver, so it is redundant initialization to set
driver.owner in dpaa2_dpio_driver statement. Remove it for clean code.
Signed-off-by: Li Zetao
---
drivers/soc/fsl/dpio/dpio-driver.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/soc/fsl/dpio/dpio-driver.c
b/drivers/soc/fsl/dpio/dpio-driver.c
index 9e3fddd8f5a9..6be4f1caafcb 100644
--- a/drivers/soc/fsl/dpio/dpio-driver.c
+++ b/drivers/soc/fsl/dpio/dpio-driver.c
@@ -312,7 +312,6 @@ static const struct fsl_mc_device_id
dpaa2_dpio_match_id_table[] = {
static struct fsl_mc_driver dpaa2_dpio_driver = {
.driver = {
.name = KBUILD_MODNAME,
- .owner = THIS_MODULE,
},
.probe = dpaa2_dpio_probe,
.remove = dpaa2_dpio_remove,
--
2.34.1
Re: [PATCH v3] hwrng: Explicitly include correct DT includes
On Fri, Jul 28, 2023 at 07:48:27AM -0600, Rob Herring wrote: > The DT of_device.h and of_platform.h date back to the separate > of_platform_bus_type before it was merged into the regular platform bus. > As part of that merge prepping Arm DT support 13 years ago, they > "temporarily" include each other. They also include platform_device.h > and of.h. As a result, there's a pretty much random mix of those include > files used throughout the tree. In order to detangle these headers and > replace the implicit includes with struct declarations, users need to > explicitly include the correct includes. > > Signed-off-by: Rob Herring > --- > v3: > - Split out hw_random, ipmi and tpm > v2: > - Fix build for pic32-rng.c dropping of_match_ptr() > --- > drivers/char/hw_random/atmel-rng.c | 2 +- > drivers/char/hw_random/bcm2835-rng.c | 3 +-- > drivers/char/hw_random/ingenic-trng.c | 2 +- > drivers/char/hw_random/iproc-rng200.c | 3 +-- > drivers/char/hw_random/npcm-rng.c | 3 +-- > drivers/char/hw_random/omap-rng.c | 2 -- > drivers/char/hw_random/omap3-rom-rng.c | 1 - > drivers/char/hw_random/pasemi-rng.c| 3 +-- > drivers/char/hw_random/pic32-rng.c | 5 ++--- > drivers/char/hw_random/stm32-rng.c | 3 ++- > drivers/char/hw_random/xgene-rng.c | 5 ++--- > drivers/char/hw_random/xiphera-trng.c | 1 - > 12 files changed, 12 insertions(+), 21 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH -next 4/5] usb: musb: Remove an unnecessary NULL value
The NULL initialization of the pointers assigned by kzalloc() first is
not necessary, because if the kzalloc() failed, the pointers will be
assigned NULL, otherwise it works as usual. so remove it.
Signed-off-by: Ruan Jinjie
---
drivers/usb/musb/musb_gadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index 31c44325e828..051c6da7cf6d 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -1130,7 +1130,7 @@ static int musb_gadget_disable(struct usb_ep *ep)
struct usb_request *musb_alloc_request(struct usb_ep *ep, gfp_t gfp_flags)
{
struct musb_ep *musb_ep = to_musb_ep(ep);
- struct musb_request *request = NULL;
+ struct musb_request *request;
request = kzalloc(sizeof *request, gfp_flags);
if (!request)
--
2.34.1
[PATCH -next 5/5] USB: usbip: Remove an unnecessary NULL value
The NULL initialization of the pointers assigned by kzalloc() first is
not necessary, because if the kzalloc() failed, the pointers will be
assigned NULL, otherwise it works as usual. so remove it.
Signed-off-by: Ruan Jinjie
---
drivers/usb/usbip/vudc_dev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
index 2bc428f2e261..8e42839e6060 100644
--- a/drivers/usb/usbip/vudc_dev.c
+++ b/drivers/usb/usbip/vudc_dev.c
@@ -489,7 +489,7 @@ static void vudc_device_unusable(struct usbip_device *ud)
struct vudc_device *alloc_vudc_device(int devid)
{
- struct vudc_device *udc_dev = NULL;
+ struct vudc_device *udc_dev;
udc_dev = kzalloc(sizeof(*udc_dev), GFP_KERNEL);
if (!udc_dev)
--
2.34.1
[PATCH -next 1/5] usb: gadget: udc: Remove unnecessary NULL values
The NULL initialization of the pointers assigned by kzalloc() first is
not necessary, because if the kzalloc() failed, the pointers will be
assigned NULL, otherwise it works as usual. so remove it.
Signed-off-by: Ruan Jinjie
---
drivers/usb/gadget/udc/fsl_udc_core.c | 2 +-
drivers/usb/gadget/udc/mv_u3d_core.c | 4 ++--
drivers/usb/gadget/udc/mv_udc_core.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c
b/drivers/usb/gadget/udc/fsl_udc_core.c
index 5265ca418cde..ee5705d336e3 100644
--- a/drivers/usb/gadget/udc/fsl_udc_core.c
+++ b/drivers/usb/gadget/udc/fsl_udc_core.c
@@ -671,7 +671,7 @@ static int fsl_ep_disable(struct usb_ep *_ep)
static struct usb_request *
fsl_alloc_request(struct usb_ep *_ep, gfp_t gfp_flags)
{
- struct fsl_req *req = NULL;
+ struct fsl_req *req;
req = kzalloc(sizeof *req, gfp_flags);
if (!req)
diff --git a/drivers/usb/gadget/udc/mv_u3d_core.c
b/drivers/usb/gadget/udc/mv_u3d_core.c
index 3473048a85f5..2a421f0ff931 100644
--- a/drivers/usb/gadget/udc/mv_u3d_core.c
+++ b/drivers/usb/gadget/udc/mv_u3d_core.c
@@ -665,7 +665,7 @@ static int mv_u3d_ep_disable(struct usb_ep *_ep)
static struct usb_request *
mv_u3d_alloc_request(struct usb_ep *_ep, gfp_t gfp_flags)
{
- struct mv_u3d_req *req = NULL;
+ struct mv_u3d_req *req;
req = kzalloc(sizeof *req, gfp_flags);
if (!req)
@@ -1779,7 +1779,7 @@ static void mv_u3d_remove(struct platform_device *dev)
static int mv_u3d_probe(struct platform_device *dev)
{
- struct mv_u3d *u3d = NULL;
+ struct mv_u3d *u3d;
struct mv_usb_platform_data *pdata = dev_get_platdata(>dev);
int retval = 0;
struct resource *r;
diff --git a/drivers/usb/gadget/udc/mv_udc_core.c
b/drivers/usb/gadget/udc/mv_udc_core.c
index 79db74e2040b..d888dcda2bc8 100644
--- a/drivers/usb/gadget/udc/mv_udc_core.c
+++ b/drivers/usb/gadget/udc/mv_udc_core.c
@@ -595,7 +595,7 @@ static int mv_ep_disable(struct usb_ep *_ep)
static struct usb_request *
mv_alloc_request(struct usb_ep *_ep, gfp_t gfp_flags)
{
- struct mv_req *req = NULL;
+ struct mv_req *req;
req = kzalloc(sizeof *req, gfp_flags);
if (!req)
--
2.34.1
[PATCH -next 0/5] usb: Remove many unnecessary NULL values
The NULL initialization of the pointers assigned by kzalloc() first is not necessary, because if the kzalloc() failed, the pointers will be assigned NULL, otherwise it works as usual. so remove it. Ruan Jinjie (5): usb: gadget: udc: Remove unnecessary NULL values USB: misc: Remove unnecessary NULL values usb: chipidea: udc: Remove an unnecessary NULL value usb: musb: Remove an unnecessary NULL value USB: usbip: Remove an unnecessary NULL value drivers/usb/chipidea/udc.c| 2 +- drivers/usb/gadget/udc/fsl_udc_core.c | 2 +- drivers/usb/gadget/udc/mv_u3d_core.c | 4 ++-- drivers/usb/gadget/udc/mv_udc_core.c | 2 +- drivers/usb/misc/cypress_cy7c63.c | 2 +- drivers/usb/misc/cytherm.c| 2 +- drivers/usb/misc/usbsevseg.c | 2 +- drivers/usb/musb/musb_gadget.c| 2 +- drivers/usb/usbip/vudc_dev.c | 2 +- 9 files changed, 10 insertions(+), 10 deletions(-) -- 2.34.1
[PATCH -next 3/5] usb: chipidea: udc: Remove an unnecessary NULL value
The NULL initialization of the pointers assigned by kzalloc() first is
not necessary, because if the kzalloc() failed, the pointers will be
assigned NULL, otherwise it works as usual. so remove it.
Signed-off-by: Ruan Jinjie
---
drivers/usb/chipidea/udc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index d58355427eeb..0b7bd3c643c3 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -1463,7 +1463,7 @@ static int ep_disable(struct usb_ep *ep)
*/
static struct usb_request *ep_alloc_request(struct usb_ep *ep, gfp_t gfp_flags)
{
- struct ci_hw_req *hwreq = NULL;
+ struct ci_hw_req *hwreq;
if (ep == NULL)
return NULL;
--
2.34.1
[PATCH -next 2/5] USB: misc: Remove unnecessary NULL values
The NULL initialization of the pointers assigned by kzalloc() first is
not necessary, because if the kzalloc() failed, the pointers will be
assigned NULL, otherwise it works as usual. so remove it.
Signed-off-by: Ruan Jinjie
---
drivers/usb/misc/cypress_cy7c63.c | 2 +-
drivers/usb/misc/cytherm.c| 2 +-
drivers/usb/misc/usbsevseg.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/misc/cypress_cy7c63.c
b/drivers/usb/misc/cypress_cy7c63.c
index 14faec51d7a5..cecd7693b741 100644
--- a/drivers/usb/misc/cypress_cy7c63.c
+++ b/drivers/usb/misc/cypress_cy7c63.c
@@ -203,7 +203,7 @@ ATTRIBUTE_GROUPS(cypress);
static int cypress_probe(struct usb_interface *interface,
const struct usb_device_id *id)
{
- struct cypress *dev = NULL;
+ struct cypress *dev;
int retval = -ENOMEM;
/* allocate memory for our device state and initialize it */
diff --git a/drivers/usb/misc/cytherm.c b/drivers/usb/misc/cytherm.c
index 3e3802aaefa3..918833b471ea 100644
--- a/drivers/usb/misc/cytherm.c
+++ b/drivers/usb/misc/cytherm.c
@@ -304,7 +304,7 @@ static int cytherm_probe(struct usb_interface *interface,
const struct usb_device_id *id)
{
struct usb_device *udev = interface_to_usbdev(interface);
- struct usb_cytherm *dev = NULL;
+ struct usb_cytherm *dev;
int retval = -ENOMEM;
dev = kzalloc (sizeof(struct usb_cytherm), GFP_KERNEL);
diff --git a/drivers/usb/misc/usbsevseg.c b/drivers/usb/misc/usbsevseg.c
index c3114d9bd128..546deff754ba 100644
--- a/drivers/usb/misc/usbsevseg.c
+++ b/drivers/usb/misc/usbsevseg.c
@@ -305,7 +305,7 @@ static int sevseg_probe(struct usb_interface *interface,
const struct usb_device_id *id)
{
struct usb_device *udev = interface_to_usbdev(interface);
- struct usb_sevsegdev *mydev = NULL;
+ struct usb_sevsegdev *mydev;
int rc = -ENOMEM;
mydev = kzalloc(sizeof(struct usb_sevsegdev), GFP_KERNEL);
--
2.34.1
Re: [PATCH -next 2/2] soc: xilinx: Do not check for 0 return after calling platform_get_irq()
On 8/3/23 12:48, Ruan Jinjie wrote:
There is no possible for platform_get_irq() to
return 0. Use the return value from platform_get_irq().
Signed-off-by: Ruan Jinjie
---
drivers/soc/xilinx/zynqmp_power.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/soc/xilinx/zynqmp_power.c
b/drivers/soc/xilinx/zynqmp_power.c
index 641dcc958911..eddfc8141a42 100644
--- a/drivers/soc/xilinx/zynqmp_power.c
+++ b/drivers/soc/xilinx/zynqmp_power.c
@@ -242,8 +242,8 @@ static int zynqmp_pm_probe(struct platform_device *pdev)
}
} else if (of_property_present(pdev->dev.of_node, "interrupts")) {
irq = platform_get_irq(pdev, 0);
- if (irq <= 0)
- return -ENXIO;
+ if (irq < 0)
+ return irq;
ret = devm_request_threaded_irq(>dev, irq, NULL,
zynqmp_pm_isr,
Applied this 2/2 patch.
Thanks,
Michal
[RFC PATCH] cxl: Use pci_find_vsec_capability() to simplify the code
PCI core add pci_find_vsec_capability() to query VSEC. We can use that
core API to simplify the code.
The only logical change is that pci_find_vsec_capability check the
Vendor ID before finding the VSEC.
PCI spec rev 5.0 says in 7.9.5.2 Vendor-Specific Header:
VSEC ID - This field is a vendor-defined ID number that indicates the
nature and format of the VSEC structure
Software must qualify the Vendor ID before interpreting this field.
Signed-off-by: Xiongfeng Wang
---
drivers/misc/cxl/pci.c | 12 ++--
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 0ff944860dda..f3108977755d 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c
@@ -150,16 +150,8 @@ static inline resource_size_t p2_size(struct pci_dev *dev)
static int find_cxl_vsec(struct pci_dev *dev)
{
- int vsec = 0;
- u16 val;
-
- while ((vsec = pci_find_next_ext_capability(dev, vsec,
PCI_EXT_CAP_ID_VNDR))) {
- pci_read_config_word(dev, vsec + 0x4, );
- if (val == CXL_PCI_VSEC_ID)
- return vsec;
- }
- return 0;
-
+ return pci_find_vsec_capability(dev, PCI_VENDOR_ID_IBM,
+ CXL_PCI_VSEC_ID);
}
static void dump_cxl_config_space(struct pci_dev *dev)
--
2.20.1
[PATCH] powerpc/powernv/pci: use pci_dev_id() to simplify the code
PCI core API pci_dev_id() can be used to get the BDF number for a pci
device. We don't need to compose it mannually. Use pci_dev_id() to
simplify the code a little bit.
Signed-off-by: Xiongfeng Wang
---
arch/powerpc/platforms/powernv/pci-ioda.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c
b/arch/powerpc/platforms/powernv/pci-ioda.c
index cb637827bc58..28fac4770073 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -997,14 +997,14 @@ static void pnv_pci_ioda_dma_dev_setup(struct pci_dev
*pdev)
struct pnv_ioda_pe *pe;
/* Check if the BDFN for this device is associated with a PE yet */
- pe = pnv_pci_bdfn_to_pe(phb, pdev->devfn | (pdev->bus->number << 8));
+ pe = pnv_pci_bdfn_to_pe(phb, pci_dev_id(pdev));
if (!pe) {
/* VF PEs should be pre-configured in pnv_pci_sriov_enable() */
if (WARN_ON(pdev->is_virtfn))
return;
pnv_pci_configure_bus(pdev->bus);
- pe = pnv_pci_bdfn_to_pe(phb, pdev->devfn | (pdev->bus->number
<< 8));
+ pe = pnv_pci_bdfn_to_pe(phb, pci_dev_id(pdev));
pci_info(pdev, "Configured PE#%x\n", pe ? pe->pe_number :
0xf);
@@ -2526,7 +2526,7 @@ static struct iommu_group *pnv_pci_device_group(struct
pci_controller *hose,
if (WARN_ON(!phb))
return ERR_PTR(-ENODEV);
- pe = pnv_pci_bdfn_to_pe(phb, pdev->devfn | (pdev->bus->number << 8));
+ pe = pnv_pci_bdfn_to_pe(phb, pci_dev_id(pdev));
if (!pe)
return ERR_PTR(-ENODEV);
--
2.20.1
Re: [PATCH v3 1/2] nmi_backtrace: Allow excluding an arbitrary CPU
On Thu 03-08-23 16:07:57, Douglas Anderson wrote:
> The APIs that allow backtracing across CPUs have always had a way to
> exclude the current CPU. This convenience means callers didn't need to
> find a place to allocate a CPU mask just to handle the common case.
>
> Let's extend the API to take a CPU ID to exclude instead of just a
> boolean. This isn't any more complex for the API to handle and allows
> the hardlockup detector to exclude a different CPU (the one it already
> did a trace for) without needing to find space for a CPU mask.
>
> Arguably, this new API also encourages safer behavior. Specifically if
> the caller wants to avoid tracing the current CPU (maybe because they
> already traced the current CPU) this makes it more obvious to the
> caller that they need to make sure that the current CPU ID can't
> change.
Yes, this looks like the best way forward.
It would have been slightly safer to modify arch_trigger_cpumask_backtrace
by switching arguments so that some leftovers are captured easier.
You also have this leftover
diff --git a/include/linux/nmi.h b/include/linux/nmi.h
index 00982b133dc1..9f1743ee2b28 100644
--- a/include/linux/nmi.h
+++ b/include/linux/nmi.h
@@ -190,10 +190,6 @@ static inline bool trigger_all_cpu_backtrace(void)
{
return false;
}
-static inline bool trigger_allbutself_cpu_backtrace(void)
-{
- return false;
-}
static inline bool trigger_cpumask_backtrace(struct cpumask *mask)
{
return false;
> Signed-off-by: Douglas Anderson
Anyway
Acked-by: Michal Hocko
> ---
>
> Changes in v3:
> - ("nmi_backtrace: Allow excluding an arbitrary CPU") new for v3.
>
> arch/arm/include/asm/irq.h | 2 +-
> arch/arm/kernel/smp.c| 4 ++--
> arch/loongarch/include/asm/irq.h | 2 +-
> arch/loongarch/kernel/process.c | 4 ++--
> arch/mips/include/asm/irq.h | 2 +-
> arch/mips/kernel/process.c | 4 ++--
> arch/powerpc/include/asm/irq.h | 2 +-
> arch/powerpc/kernel/stacktrace.c | 4 ++--
> arch/powerpc/kernel/watchdog.c | 4 ++--
> arch/sparc/include/asm/irq_64.h | 2 +-
> arch/sparc/kernel/process_64.c | 6 +++---
> arch/x86/include/asm/irq.h | 2 +-
> arch/x86/kernel/apic/hw_nmi.c| 4 ++--
> include/linux/nmi.h | 12 ++--
> kernel/watchdog.c| 2 +-
> lib/nmi_backtrace.c | 6 +++---
> 16 files changed, 31 insertions(+), 31 deletions(-)
>
> diff --git a/arch/arm/include/asm/irq.h b/arch/arm/include/asm/irq.h
> index 18605f1b3580..26c1d2ced4ce 100644
> --- a/arch/arm/include/asm/irq.h
> +++ b/arch/arm/include/asm/irq.h
> @@ -32,7 +32,7 @@ void handle_IRQ(unsigned int, struct pt_regs *);
> #include
>
> extern void arch_trigger_cpumask_backtrace(const cpumask_t *mask,
> -bool exclude_self);
> +int exclude_cpu);
> #define arch_trigger_cpumask_backtrace arch_trigger_cpumask_backtrace
> #endif
>
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 6756203e45f3..3431c0553f45 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -846,7 +846,7 @@ static void raise_nmi(cpumask_t *mask)
> __ipi_send_mask(ipi_desc[IPI_CPU_BACKTRACE], mask);
> }
>
> -void arch_trigger_cpumask_backtrace(const cpumask_t *mask, bool exclude_self)
> +void arch_trigger_cpumask_backtrace(const cpumask_t *mask, int exclude_cpu)
> {
> - nmi_trigger_cpumask_backtrace(mask, exclude_self, raise_nmi);
> + nmi_trigger_cpumask_backtrace(mask, exclude_cpu, raise_nmi);
> }
> diff --git a/arch/loongarch/include/asm/irq.h
> b/arch/loongarch/include/asm/irq.h
> index a115e8999c69..218b4da0ea90 100644
> --- a/arch/loongarch/include/asm/irq.h
> +++ b/arch/loongarch/include/asm/irq.h
> @@ -40,7 +40,7 @@ void spurious_interrupt(void);
> #define NR_IRQS_LEGACY 16
>
> #define arch_trigger_cpumask_backtrace arch_trigger_cpumask_backtrace
> -void arch_trigger_cpumask_backtrace(const struct cpumask *mask, bool
> exclude_self);
> +void arch_trigger_cpumask_backtrace(const struct cpumask *mask, int
> exclude_cpu);
>
> #define MAX_IO_PICS 2
> #define NR_IRQS (64 + (256 * MAX_IO_PICS))
> diff --git a/arch/loongarch/kernel/process.c b/arch/loongarch/kernel/process.c
> index 2e04eb07abb6..778e8d09953e 100644
> --- a/arch/loongarch/kernel/process.c
> +++ b/arch/loongarch/kernel/process.c
> @@ -345,9 +345,9 @@ static void raise_backtrace(cpumask_t *mask)
> }
> }
>
> -void arch_trigger_cpumask_backtrace(const cpumask_t *mask, bool exclude_self)
> +void arch_trigger_cpumask_backtrace(const cpumask_t *mask, int exclude_cpu)
> {
> - nmi_trigger_cpumask_backtrace(mask, exclude_self, raise_backtrace);
> + nmi_trigger_cpumask_backtrace(mask, exclude_cpu, raise_backtrace);
> }
>
> #ifdef CONFIG_64BIT
> diff --git a/arch/mips/include/asm/irq.h b/arch/mips/include/asm/irq.h
> index 75abfa834ab7..3a848e7e69f7 100644
> ---
