Re: SPDX identifier
Arend, On Thu, 16 May 2019, Arend Van Spriel wrote: > Hi Kalle, Thomas, > > I added SPDX tags in brcm80211 driver sources. Although it is a no-brainer I > decided to run checkpatch for the changes and quirky stuff started to happen. > For all files I added: > > // SPDX-License-Identifier > > but checkpatch started complaining I should use /* ... */ instead of //. > > WARNING: Improper SPDX comment style for > 'drivers/net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h', please use > '/*' instead > #29: FILE: drivers/net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h:1: > +// SPDX-License-Identifier: ISC > > So I edited all patches and ran again. And again it started complaining. > > WARNING: Improper SPDX comment style for > 'drivers/net/wireless/broadcom/brcm80211/brcmsmac/aiutils.c', please use '//' > instead > > So now I am in a bonkers state. It seems for header files we want /* */ and > for c files we want //. For real? See Documentation. This is historical because the older binutils choked on '//' comments. Not longer an issue as we moved to more modern binutils by now. So we can fixup the documentation and allow // style for headers as well. Thanks, tglx
Re: [PATCH v4 31/36] mac80211_hwsim: Replace hrtimer tasklet with softirq hrtimer
On Thu, 4 Jan 2018, Johannes Berg wrote: > On Thu, 2017-12-21 at 11:42 +0100, Anna-Maria Gleixner wrote: > > From: Thomas Gleixner > > > > Switch the timer to HRTIMER_MODE_SOFT, which executed the timer > > I've pointed out before that this should say HRTIMER_MODE_REL_SOFT. > > Anyway, since it still doesn't apply on my tree, somebody else will > have to pick it up. Yes. I'm going to do that or decide to postpone the conversion patches. Have not yet thought about it as I was distracted for the last 2 month by something unpleasant Thanks, tglx
Re: 915f3e3f76 ("mac80211_hwsim: Replace bogus hrtimer clockid"): BUG: kernel reboot-without-warning in test stage
On Tue, 5 Sep 2017, kernel test robot wrote: > Greetings, > > 0day kernel testing robot got the below dmesg and the first bad commit is > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master > > commit 915f3e3f76c05b2da93c4cc278eebc2d9219d9f4 > Author: Thomas Gleixner > AuthorDate: Sat Feb 25 11:27:37 2017 +0100 > Commit: Linus Torvalds > CommitDate: Sat Feb 25 09:48:16 2017 -0800 > > mac80211_hwsim: Replace bogus hrtimer clockid > > mac80211_hwsim initializes a hrtimer with clockid CLOCK_MONOTONIC_RAW. > That's not supported. > > Use CLOCK_MONOTNIC instead. Sorry, no. That bisect is completely bogus. The commit in question merily replaces the unsupported clockid with a valid one. Thanks, tglx
Re: [PATCH 20/25] mac80211_hwsim: Replace hrtimer tasklet with softirq hrtimer
On Tue, 5 Sep 2017, Johannes Berg wrote: > On Thu, 2017-08-31 at 12:23 +, Anna-Maria Gleixner wrote: > > From: Thomas Gleixner > > > > Switch the timer to CLOCK_MONOTONIC_SOFT, which executed the timer > > callback in softirq context and remove the hrtimer_tasklet. > > > > Signed-off-by: Thomas Gleixner > > Signed-off-by: Anna-Maria Gleixner > > Cc: Johannes Berg > > Cc: Kalle Valo > > Cc: linux-wireless@vger.kernel.org > > > This looks fine to me, > > Reviewed-by: Johannes Berg > > Are you planning to integrate all patches in the series through some > other tree, perhaps to be able to get rid of the tasklet_hrtimer API, > or should I apply this? The patch depends on the hrtimer core changes, so we either delay the removal for a release cycle or just take the whole lot through tip:timers/core and get rid of it in one go. Thanks, tglx
Re: 915f3e3f76 ("mac80211_hwsim: Replace bogus hrtimer clockid"): BUG: kernel reboot-without-warning in test stage
On Tue, 5 Sep 2017, Sebastian Andrzej Siewior wrote: > On 2017-09-05 09:12:40 [+0200], Thomas Gleixner wrote: > > Sorry, no. That bisect is completely bogus. The commit in question merily > > replaces the unsupported clockid with a valid one. > > The bisect is correct. It just has problems to express itself properly. So > the table says: > > | WARNING:at_kernel/time/hrtimer.c:#hrtimer_init | 7 | || | > > | BUG:kernel_reboot-without-warning_in_test_stage | 0 | 12 | 6 | 6 | > > > which means _before_ your commit it counted a warning in hrtimer_init() > (an unsupported clock id was used). With your commit, the warning was > gone and I *think* the userland then printed > "BUG:kernel_reboot-without-warning_in_test_stage" because it had no > warning. > It seems that the bot learned to live with that warning which was around > for more than three years. Now that you removed it, it seems to be a > mistake to do so because nobody complained about it so far. Thanks for the translation. I'll never learn to decode these reports. tglx
Re: [PATCH] PCI MSI: allow alignment restrictions on vector allocation
On Mon, 25 Sep 2017, Daniel Drake wrote:
Please send x86 related patches to LKML as documented in MAINTAINERS. That
patch is mainly x86 and not PCI.
> ath9k hardware claims to support up to 4 MSI vectors, and when run in
> that configuration, it would be allowed to modify the lower bits of the
> MSI Message Data when generating interrupts in order to signal which
> of the 4 vectors the interrupt is being raised on.
>
> Linux's PCI-MSI irqchip only supports a single MSI vector for each
> device, and it tells the device this, but the device appears to assume
> it is working with 4, as it will unset the lower 2 bits of Message Data
> presumably to indicate that it is an IRQ for the first of 4 possible
> vectors.
>
> Linux will then receive an interrupt on the wrong vector, so the
> ath9k interrupt handler will not be invoked.
>
> To work around this, introduce a mechanism where the vector assignment
> algorithm can be restricted to only a subset of available vector numbers
> based on a bitmap.
>
> As a user of this bitmap, introduce a pci_dev.align_msi_vector flag which
> can be used to state that MSI vector numbers must be aligned to a specific
> amount. If we 4-align the ath9k MSI vector then the lower bits will
> already be 0 and hence the device will not modify the Message Data away
> from its original value.
>
> This is needed in order to support the wifi card in at least 8 new Acer
> consumer laptop models which come with the Foxconn NFA335 WiFi module.
> Legacy interrupts do not work on that module, so MSI support is required.
Sigh, what a mess.
> diff --git a/arch/x86/include/asm/hw_irq.h b/arch/x86/include/asm/hw_irq.h
> index 6dfe366a8804..7f35178586a1 100644
> --- a/arch/x86/include/asm/hw_irq.h
> +++ b/arch/x86/include/asm/hw_irq.h
> @@ -77,6 +77,7 @@ struct irq_alloc_info {
> struct {
> struct pci_dev *msi_dev;
> irq_hw_number_t msi_hwirq;
> + DECLARE_BITMAP(allowed_vectors, NR_VECTORS);
Uurgh. No. You want to express an alignment requirement. Why would you need
a bitmap for that? A simple
unsigned int align_vector;
is sufficient.
> @@ -178,6 +179,9 @@ static int __assign_irq_vector(int irq, struct
> apic_chip_data *d,
> if (test_bit(vector, used_vectors))
> goto next;
>
> + if (allowed_vectors && !test_bit(vector, allowed_vectors))
> + goto next;
This code is gone in -next and replaced by a new vector allocator.
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/apic
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index f68c58a93dd0..7755450be02d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -419,6 +419,8 @@ struct pci_dev {
> #endif
> #ifdef CONFIG_PCI_MSI
> const struct attribute_group **msi_irq_groups;
> + int align_msi_vector; /* device requires MSI vector numbers aligned
> + * by this amount */
This wants to be unsigned int and please get rid of this butt ugly
formatted comment.
But the real question is how this is supposed to work with
- interrupt remapping
- non X86 machines
I doubt that this can be made generic enough to make it work all over the
place. Tell Acer/Foxconn to fix their stupid firmware.
Thanks,
tglx
Re: [PATCH] PCI MSI: allow alignment restrictions on vector allocation
Daniel, On Mon, 2 Oct 2017, Daniel Drake wrote: > On Wed, Sep 27, 2017 at 11:28 PM, Thomas Gleixner wrote: > On another system, I have multiple devices using IR-PCI-MSI according > to /proc/interrupts, and lspci shows that a MSI Message Data value 0 > is used for every single MSI-enabled device. > > I don't know if that's just luck, but its a value that would work > fine for ath9k. It's an implementation detail... > After checking out the new code and thinking this through a bit, I think > perhaps the only generic approach that would work is to make the > ath9k driver require a vector allocation that enables the entire block > of 4 MSI IRQs that the hardware supports (which is what Windows is doing). I wonder how Windows deals with the affinity problem for multi-MSI. Or does it just not allow to set it at all? > This way the alignment requirement is automatically met and ath9k is > free to stamp all over the lower 2 bits of the MSI Message Data. > > MSI_FLAG_MULTI_PCI_MSI is already supported by a couple of non-x86 drivers > and the interrupt remapping code, but it is not supported by the generic > pci_msi_controller, and the new vector allocator still rejects > X86_IRQ_ALLOC_CONTIGUOUS_VECTORS. Yes, and it does so because Multi-MSI on x86 requires single destination for all vectors, that means the affinity is the same for all vectors. This has two implications: 1) The generic interrupt code and its affinity management are not able to deal with that. Probably a solvable problem, but not trivial either. 2) The affinity setting of straight MSI interrupts (w/o remapping) on x86 requires to make the affinity change from the interrupt context of the current active vector in order not to lose interrupts or worst case getting into a stale state. That works for single vectors, but trying to move all vectors in one go is more or less impossible, as there is no reliable way to determine that none of the other vectors is on flight. There might be some 'workarounds' for that, but I rather avoid that unless we get an official documented one from Intel/AMD. With interrupt remapping this is a different story as the remapping unit removes all these problems and things just work. The switch to best effort reservation mode for vectors is another interesting problem. This cannot work with non remapped multi-MSI, unless we just give up for these type of interrupts and associate them straight away. I could be persuaded to do so, but the above problems (especially #2) wont magically go away. > I will try to take a look at enabling this for the generic allocator, > unless you have any other ideas. See above. > In the mean time, at least for reference, below is an updated version > of the previous patch based on the new allocator and incorporating > your feedback. (It's still rather ugly though) > > > I doubt that this can be made generic enough to make it work all over the > > place. Tell Acer/Foxconn to fix their stupid firmware. > > We're also working on this approach ever since the problematic models > first appeared months ago, but since these products are in mass production, I really wonder how they managed to screw that up. The spec is pretty strict about that: "The Multiple Message Enable field (bits 6-4 of the Message Control register) defines the number of low order message data bits the function is permitted to modify to generate its system software allocated vectors. For example, a Multiple Message Enable encoding of “010” indicates the function has been allocated four vectors and is permitted to modify message data bits 1 and 0 (a function modifies the lower message data bits to generate the allocated number of vectors). If the Multiple Message Enable field is “000”, the function is not permitted to modify the message data." Not permitted is the keyword here. > I think ultimately we are going to need a Linux workaround. What's wrong with just using the legacy INTx emulation if you cannot allocate 4 MSI vectors? Thanks, tglx
Re: [PATCH] PCI MSI: allow alignment restrictions on vector allocation
On Mon, 2 Oct 2017, Thomas Gleixner wrote: > On Mon, 2 Oct 2017, Daniel Drake wrote: > 2) The affinity setting of straight MSI interrupts (w/o remapping) on x86 > requires to make the affinity change from the interrupt context of the > current active vector in order not to lose interrupts or worst case > getting into a stale state. > > That works for single vectors, but trying to move all vectors in one > go is more or less impossible, as there is no reliable way to > determine that none of the other vectors is on flight. > > There might be some 'workarounds' for that, but I rather avoid that > unless we get an official documented one from Intel/AMD. Thinking more about it. That might be actually a non issue for MSI, but we have that modus operandi in the current code and we need to address that first before even thinking about multi MSI support. Thanks, tglx
Re: [PATCH] PCI MSI: allow alignment restrictions on vector allocation
On Mon, 2 Oct 2017, Thomas Gleixner wrote: > On Mon, 2 Oct 2017, Thomas Gleixner wrote: > > On Mon, 2 Oct 2017, Daniel Drake wrote: > > 2) The affinity setting of straight MSI interrupts (w/o remapping) on x86 > > requires to make the affinity change from the interrupt context of the > > current active vector in order not to lose interrupts or worst case > > getting into a stale state. > > > > That works for single vectors, but trying to move all vectors in one > > go is more or less impossible, as there is no reliable way to > > determine that none of the other vectors is on flight. > > > > There might be some 'workarounds' for that, but I rather avoid that > > unless we get an official documented one from Intel/AMD. > > Thinking more about it. That might be actually a non issue for MSI, but we > have that modus operandi in the current code and we need to address that > first before even thinking about multi MSI support. But even if its possible, it's very debatable whether it's worth the effort when this driver just can use the legacy INTx.and be done with it. Thanks, tglx
Re: [PATCH] PCI MSI: allow alignment restrictions on vector allocation
On Tue, 3 Oct 2017, Bjorn Helgaas wrote: > On Tue, Oct 03, 2017 at 11:07:58PM +0200, Thomas Gleixner wrote: > > On Mon, 2 Oct 2017, Thomas Gleixner wrote: > > > On Mon, 2 Oct 2017, Thomas Gleixner wrote: > > > > On Mon, 2 Oct 2017, Daniel Drake wrote: > > > > 2) The affinity setting of straight MSI interrupts (w/o remapping) on > > > > x86 > > > > requires to make the affinity change from the interrupt context of > > > > the > > > > current active vector in order not to lose interrupts or worst case > > > > getting into a stale state. > > > > > > > > That works for single vectors, but trying to move all vectors in > > > > one > > > > go is more or less impossible, as there is no reliable way to > > > > determine that none of the other vectors is on flight. > > > > > > > > There might be some 'workarounds' for that, but I rather avoid that > > > > unless we get an official documented one from Intel/AMD. > > > > > > Thinking more about it. That might be actually a non issue for MSI, but we > > > have that modus operandi in the current code and we need to address that > > > first before even thinking about multi MSI support. > > > > But even if its possible, it's very debatable whether it's worth the effort > > when this driver just can use the legacy INTx.and be done with it. > > Daniel said "Legacy interrupts do not work on that module, so MSI > support is required," so I assume he means INTx doesn't work. Maybe Hmm, I missed that detail obviously. > the driver could poll? I don't know how much slower that would be, > but at least it would penalize the broken device, not everybody. That would definitely be prefered. Thanks, tglx
Re: [PATCH] PCI MSI: allow alignment restrictions on vector allocation
On Thu, 5 Oct 2017, Daniel Drake wrote: > On Mon, Oct 2, 2017 at 10:38 PM, Thomas Gleixner wrote: > > What's wrong with just using the legacy INTx emulation if you cannot > > allocate 4 MSI vectors? > > The Legacy interrupt simply doesn't work for the wifi on at least 8 new Acer > laptop products based on Intel Apollo Lake. > Plus 4 Dell systems included in the patches in this thread: > https://lkml.org/lkml/2017/9/26/55 > (the 2 which I can find specs for are also Apollo Lake) > > We have tried taking the mini-PCIe wifi module out of one of the affected > Acer products and moved it to another computer, where it is working fine > with legacy interrupts. So this suggests that the wifi module itself is OK, > but we are facing a hardware limitation or BIOS limitation on the affected > products. In the Dell thread it says "Some platform(BIOS) blocks legacy > interrupts (INTx)". > > If you have any suggestions for how we might solve this without getting into > the MSI mess then that would be much appreciated. If the BIOS blocks the > interrupts, can Linux unblock them? I'm pretty sure we can. Cc'ed Rafael and Andy. They might know, if not they certainly know whom to ask @Intel. > Just for reference I'm attaching my latest attempt at enabling MULTI_PCI_MSI. > It would definitely need further work if we proceed here - so far I've > ignored the affinity considerations that you explained, and it's not > particularly clean. Yeah, I know how that looks. When I rewrote the allocator I initialy had that multi-vector mode implemented and then ditched it due to the affinity setting mess and because its hard vs. the global best effort reservation mode, which is way more important to have than multi MSI. > int irq_matrix_alloc(struct irq_matrix *m, const struct cpumask *msk, > - bool reserved, unsigned int *mapped_cpu); > + bool reserved, unsigned int *mapped_cpu, unsigned int num, > + unsigned int align_mask); That's not needed. We can assume that multivector allocations must be aligned in the following way: count = __roundup_pow_of_two(count); mask = ilog2(count); That's a sane assumption in general. Thanks, tglx
Re: [PATCH v2 31/37] mac80211_hwsim: Replace hrtimer tasklet with softirq hrtimer
On Mon, 23 Oct 2017, Johannes Berg wrote: > On Sun, 2017-10-22 at 23:40 +0200, Anna-Maria Gleixner wrote: > > From: Thomas Gleixner > > > > Switch the timer to HRTIMER_MODE_SOFT, which executed the timer > > callback in softirq context and remove the hrtimer_tasklet. I can't build as its part of the whole hrtimer rework series. > This doesn't build on my tree, due to HRTIMER_MODE_REL_SOFT not > existing. Neither does HRTIMER_MODE_SOFT mentioned above, but that's > just a commit log mistake. Oops. > (It also didn't apply on my tree, but that was a trivial context > change.) > > I don't understand why you initialize it with HRTIMER_MODE_ABS_SOFT > either though: > > > + hrtimer_init(&data->beacon_timer, CLOCK_MONOTONIC, > > +HRTIMER_MODE_ABS_SOFT); Sure, though it does not matter in that case. Will fix that up. Thanks, tglx
Re: [PATCH v2 31/37] mac80211_hwsim: Replace hrtimer tasklet with softirq hrtimer
On Mon, 23 Oct 2017, Johannes Berg wrote: > On Mon, 2017-10-23 at 12:23 +0200, Thomas Gleixner wrote: > > On Mon, 23 Oct 2017, Johannes Berg wrote: > > > > > On Sun, 2017-10-22 at 23:40 +0200, Anna-Maria Gleixner wrote: > > > > From: Thomas Gleixner > > > > > > > > Switch the timer to HRTIMER_MODE_SOFT, which executed the timer > > > > callback in softirq context and remove the hrtimer_tasklet. > > > > I can't build as its part of the whole hrtimer rework series. > > I guess you mean you *can* build it? Surely you're introducing the new > HR timer modes in some patch that I didn't see? :-) Sorry, we did not want to expose you to 30 patches fiddling with the core code. They are on LKML though. > > > > + hrtimer_init(&data->beacon_timer, CLOCK_MONOTONIC, > > > > +HRTIMER_MODE_ABS_SOFT); > > > > Sure, though it does not matter in that case. Will fix that up. > > Right. Then again, why even pass it to init() and start()? Can you > start without going through start()? There is a subtle magic with CLOCK_REALTIME timers. CLOCK_REALTIME timers differentiate between ABS and REL modes. ABS timers are exposed to clock modifications (settimeofday() ...), REL timers are not. We solve that by associating them to different clock bases, which has to be done at init time, but the start function needs the REL/ABS information as well. For CLOCK_MONOTONIC this is not really required, but the function is used for all clock bases, so we require the mode bits for all. Thanks, tglx
Re: [run_timer_softirq] BUG: unable to handle kernel paging request at 0000000000010007
On Fri, 10 Nov 2017, Linus Torvalds wrote: > On Wed, Nov 8, 2017 at 9:19 PM, Fengguang Wu wrote: > > > > Yes it's accessing the list. Here is the faddr2line output. > > Ok, so it's a corrupted timer list. Which is not a big surprise. > > It's > > next->pprev = pprev; > > in __hlist_del(), and the trapping instruction decodes as > > mov%rdx,0x8(%rax) > > with %rax having the value dead0200, > > Which is just LIST_POISON2. > > So we've deleted that entry twice - LIST_POISON2 is what hlist_del() > sets pprev to after already deleting it once. > > Although in this case it might not be hlist_del(), because > detach_timer() also sets entry->next to LIST_POISON2. > > Which is pretty bogus, we are supposed to use LIST_POISON1 for the > "next" pointer. Oh well. Nobody cares, except for the list entry > debugging code, which isn't run on the hlist cases. > > Adding Thomas Gleixner to the cc. It should not be possible to delete > the same timer twice. Right, it shouldn't. Fengguang, can you please enable: CONFIG_DEBUG_OBJECTS CONFIG_DEBUG_OBJECTS_TIMERS and try to reproduce? Debugobject should catch that hopefully. Thanks, tglx
Re: [linux-stable] 4fc2942b6e kernel BUG at kernel/time/hrtimer.c:109!
On Fri, 17 Feb 2017, kernel test robot wrote: > Hi Marc, > > We find this oops in linux-4.4.y. The gcc-6 compiled mainline kernel is fine. > > commit 4fc2942b6e2de2efc8a9d3784d4b0d3543149613 > hrtimer: Catch illegal clockids And that commit is doing what the subject line says. Catch illegal usage. > [ 38.101342] Call Trace: > [ 38.101342] Call Trace: > [ 38.102045] [] tasklet_hrtimer_init+0x16/0x52 > [ 38.102045] [] tasklet_hrtimer_init+0x16/0x52 > [ 38.103698] [] mac80211_hwsim_new_radio+0x766/0x84d The real bug is in this code: drivers/net/wireless/mac80211_hwsim.c mac80211_hwsim_new_radio() tasklet_hrtimer_init(&data->beacon_timer, mac80211_hwsim_beacon, CLOCK_MONOTONIC_RAW, HRTIMER_MODE_ABS); CLOCK_MONOTONIC_RAW is not a supported clockid for hrtimers. Sigh. Fix below. Thanks, tglx 8<-- diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c index 0cd95120bc78..da363ec91a1c 100644 --- a/drivers/net/wireless/mac80211_hwsim.c +++ b/drivers/net/wireless/mac80211_hwsim.c @@ -2535,9 +2535,8 @@ static int mac80211_hwsim_new_radio(struct genl_info *info, data->debugfs, data, &hwsim_simulate_radar); - tasklet_hrtimer_init(&data->beacon_timer, -mac80211_hwsim_beacon, -CLOCK_MONOTONIC_RAW, HRTIMER_MODE_ABS); + tasklet_hrtimer_init(&data->beacon_timer, mac80211_hwsim_beacon, +CLOCK_MONOTONIC, HRTIMER_MODE_ABS); spin_lock_bh(&hwsim_radio_lock); list_add_tail(&data->list, &hwsim_radios);
Re: [linux-stable] 4fc2942b6e kernel BUG at kernel/time/hrtimer.c:109!
On Fri, 17 Feb 2017, Marc Zyngier wrote: > On 17/02/17 09:28, Thomas Gleixner wrote: > > On Fri, 17 Feb 2017, kernel test robot wrote: > > > >> Hi Marc, > >> > >> We find this oops in linux-4.4.y. The gcc-6 compiled mainline kernel is > >> fine. > > The last bit is worrying, as mainline has the exact same bug. Has it > been tested the same way? The thing is that we reverted that patch in mainline: See: commit 82e88ff1ea948d83125a8aaa7c9809f03ccc500f I don't know why I reverted that as well. I should have kept it. Darn. I cherry pick it and send it linus wards next week. Thanks, tglx
