* Re: [RFC PATCH 17/23] watchdog/hardlockup/hpet: Convert the timer's interrupt to NMI
From: Ricardo Neri @ 2018-06-20 0:15 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Ingo Molnar, H. Peter Anvin, Andi Kleen, Ashok Raj,
Borislav Petkov, Tony Luck, Ravi V. Shankar, x86, sparclinux,
linuxppc-dev, linux-kernel, Jacob Pan, Rafael J. Wysocki,
Don Zickus, Nicholas Piggin, Michael Ellerman,
Frederic Weisbecker, Alexei Starovoitov, Babu Moger,
Mathieu Desnoyers, Masami Hiramatsu, Peter Zijlstra,
Andrew Morton, Philippe Ombredanne, Colin Ian King,
Byungchul Park, Paul E. McKenney, Luis R. Rodriguez, Waiman Long,
Josh Poimboeuf, Randy Dunlap, Davidlohr Bueso, Christoffer Dall,
Marc Zyngier, Kai-Heng Feng, Konrad Rzeszutek Wilk,
David Rientjes, iommu
In-Reply-To: <alpine.DEB.2.21.1806161517050.1582@nanos.tec.linutronix.de>
On Sat, Jun 16, 2018 at 03:24:49PM +0200, Thomas Gleixner wrote:
> On Fri, 15 Jun 2018, Ricardo Neri wrote:
> > On Fri, Jun 15, 2018 at 11:19:09AM +0200, Thomas Gleixner wrote:
> > > On Thu, 14 Jun 2018, Ricardo Neri wrote:
> > > > Alternatively, there could be a counter that skips reading the HPET status
> > > > register (and the detection of hardlockups) for every X NMIs. This would
> > > > reduce the overall frequency of HPET register reads.
> > >
> > > Great plan. So if the watchdog is the only NMI (because perf is off) then
> > > you delay the watchdog detection by that count.
> >
> > OK. This was a bad idea. Then, is it acceptable to have an read to an HPET
> > register per NMI just to check in the status register if the HPET timer
> > caused the NMI?
>
> The status register is useless in case of MSI. MSI is edge triggered ....
>
> The only register which gives you proper information is the counter
> register itself. That adds an massive overhead to each NMI, because the
> counter register access is synchronized to the HPET clock with hardware
> magic. Plus on larger systems, the HPET access is cross node and even
> slower.
It starts to sound that the HPET is too slow to drive the hardlockup detector.
Would it be possible to envision a variant of this implementation? In this
variant, the HPET only targets a single CPU. The actual hardlockup detector
is implemented by this single CPU sending interprocessor interrupts to the
rest of the CPUs.
In this manner only one CPU has to deal with the slowness of the HPET; the
rest of the CPUs don't have to read or write any HPET registers. A sysfs
entry could be added to configure which CPU will have to deal with the HPET
timer. However, profiling could not be done accurately on such CPU.
Thanks and BR,
Ricardo
^ permalink raw reply
* Re: [git pull] Please pull powerpc.git next branch
From: Benjamin Herrenschmidt @ 2018-06-20 0:02 UTC (permalink / raw)
To: Linus Torvalds
Cc: linuxppc-dev list, Andrew Morton, Linux Kernel Mailing List
In-Reply-To: <CA+55aFxRyp4G4niZ=aMk+epdSbu5MB1oJg_h4NhLkxsM+f+SLQ@mail.gmail.com>
On Wed, 2018-06-20 at 07:58 +0900, Linus Torvalds wrote:
> On Wed, Jun 20, 2018 at 5:23 AM Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc.git merge
>
> I love the alleged line removal, but there's nothing in that 'merge'
> branch. It points to an ancient powerpc commit from 2016.
>
> And please, signed tags. I know you do them, because you used to send me them.
>
I didn't send this. It's an ancient (8 years old) email... I have no
idea how it got resent while I was asleep...
Cheers,
Ben.
^ permalink raw reply
* Re: Constant ata messages on console with commit 28361c403683 ("libata: add extra internal command") (was Re: [GIT PULL 2/2] libata changes for v4.18-rc1)
From: Jens Axboe @ 2018-06-19 23:38 UTC (permalink / raw)
To: Michael Ellerman, Tejun Heo
Cc: linux-kernel, linux-ide, Linus Torvalds, linuxppc-dev
In-Reply-To: <87r2l2wdrc.fsf@concordia.ellerman.id.au>
On 6/19/18 5:35 PM, Michael Ellerman wrote:
> Jens Axboe <axboe@kernel.dk> writes:
>> On 6/19/18 1:29 AM, Michael Ellerman wrote:
>>> Jens Axboe <axboe@kernel.dk> writes:
>>>> On 6/18/18 1:33 AM, Michael Ellerman wrote:
>>>>> Tejun Heo <tj@kernel.org> writes:
>>>>> ...
>>>>>> Jens Axboe (10):
>>>>>> libata: introduce notion of separate hardware tags
>>>>>> libata: convert core and drivers to ->hw_tag usage
>>>>>> libata: bump ->qc_active to a 64-bit type
>>>>>> libata: use ata_tag_internal() consistently
>>>>>> libata: remove assumption that ATA_MAX_QUEUE - 1 is the max
>>>>>> sata_nv: set host can_queue count appropriately
>>>>>> libata: add extra internal command
>>>>>
>>>>> Replying here because I can't find the original mail.
>>>>>
>>>>> The above commit is causing one of my machines to constantly spew ata
>>>>> messages on the console, according to bisect:
>>>>>
>>>>> # first bad commit: [28361c403683c2b00d4f5e76045f3ccd299bf99d] libata: add extra internal command
>>>>>
>>>>> To get it to boot I have to also apply:
>>>>>
>>>>> 88e10092f6a6 ("sata_fsl: use the right type for tag bitshift")
>>>>>
>>>>>
>>>>> The system boots OK and seems fine, except that it's just printing
>>>>> multiple of these per second:
>>>>>
>>>>> ata2: Signature Update detected @ 0 msecs
>>>>> ata2: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>>>>> ata2.00: configured for UDMA/100
>>>>> ata2: Signature Update detected @ 0 msecs
>>>>> ata2: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>>>>> ata2.00: configured for UDMA/100
>>>>> ata2: Signature Update detected @ 0 msecs
>>>>> ata2: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>>>>> ata2.00: configured for UDMA/100
>>>>> ata2: Signature Update detected @ 0 msecs
>>>>> ata2: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>>>>> ata2.00: configured for UDMA/100
>>>>> ata2: Signature Update detected @ 0 msecs
> ...
>>
>> Actually, just try this one on top of current -git.
>
> Yep that fixes it.
>
> No more message spam, and when I try to mount sr0 it says "no medium".
>
> I'll have to go into the office to actually put a disc in the drive
> to check it's really working, but it seems likely.
>
> Thanks for debugging it, here's a tested-by if you like:
>
> Tested-by: Michael Ellerman <mpe@ellerman.id.au>
Thanks, but I packaged it a little differently, see the other
series I CC'ed you on. It'll work the same, though. It's in Tejun's
tree now, so should end up with Linus some time this week I think.
Thanks for reporting and testing the fix!
--
Jens Axboe
^ permalink raw reply
* Re: Constant ata messages on console with commit 28361c403683 ("libata: add extra internal command") (was Re: [GIT PULL 2/2] libata changes for v4.18-rc1)
From: Michael Ellerman @ 2018-06-19 23:35 UTC (permalink / raw)
To: Jens Axboe, Tejun Heo
Cc: linux-kernel, linux-ide, Linus Torvalds, linuxppc-dev
In-Reply-To: <f82edaa2-4b1a-c747-3d58-ec86ceb4ee7e@kernel.dk>
Jens Axboe <axboe@kernel.dk> writes:
> On 6/19/18 1:29 AM, Michael Ellerman wrote:
>> Jens Axboe <axboe@kernel.dk> writes:
>>> On 6/18/18 1:33 AM, Michael Ellerman wrote:
>>>> Tejun Heo <tj@kernel.org> writes:
>>>> ...
>>>>> Jens Axboe (10):
>>>>> libata: introduce notion of separate hardware tags
>>>>> libata: convert core and drivers to ->hw_tag usage
>>>>> libata: bump ->qc_active to a 64-bit type
>>>>> libata: use ata_tag_internal() consistently
>>>>> libata: remove assumption that ATA_MAX_QUEUE - 1 is the max
>>>>> sata_nv: set host can_queue count appropriately
>>>>> libata: add extra internal command
>>>>
>>>> Replying here because I can't find the original mail.
>>>>
>>>> The above commit is causing one of my machines to constantly spew ata
>>>> messages on the console, according to bisect:
>>>>
>>>> # first bad commit: [28361c403683c2b00d4f5e76045f3ccd299bf99d] libata: add extra internal command
>>>>
>>>> To get it to boot I have to also apply:
>>>>
>>>> 88e10092f6a6 ("sata_fsl: use the right type for tag bitshift")
>>>>
>>>>
>>>> The system boots OK and seems fine, except that it's just printing
>>>> multiple of these per second:
>>>>
>>>> ata2: Signature Update detected @ 0 msecs
>>>> ata2: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>>>> ata2.00: configured for UDMA/100
>>>> ata2: Signature Update detected @ 0 msecs
>>>> ata2: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>>>> ata2.00: configured for UDMA/100
>>>> ata2: Signature Update detected @ 0 msecs
>>>> ata2: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>>>> ata2.00: configured for UDMA/100
>>>> ata2: Signature Update detected @ 0 msecs
>>>> ata2: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>>>> ata2.00: configured for UDMA/100
>>>> ata2: Signature Update detected @ 0 msecs
...
>
> Actually, just try this one on top of current -git.
Yep that fixes it.
No more message spam, and when I try to mount sr0 it says "no medium".
I'll have to go into the office to actually put a disc in the drive
to check it's really working, but it seems likely.
Thanks for debugging it, here's a tested-by if you like:
Tested-by: Michael Ellerman <mpe@ellerman.id.au>
cheers
> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
> index b8d9cfc60374..4007a9ae650d 100644
> --- a/drivers/ata/sata_fsl.c
> +++ b/drivers/ata/sata_fsl.c
> @@ -395,12 +395,6 @@ static inline unsigned int sata_fsl_tag(unsigned int tag,
> {
> /* We let libATA core do actual (queue) tag allocation */
>
> - /* all non NCQ/queued commands should have tag#0 */
> - if (ata_tag_internal(tag)) {
> - DPRINTK("mapping internal cmds to tag#0\n");
> - return 0;
> - }
> -
> if (unlikely(tag >= SATA_FSL_QUEUE_DEPTH)) {
> DPRINTK("tag %d invalid : out of range\n", tag);
> return 0;
> @@ -1229,7 +1223,7 @@ static void sata_fsl_host_intr(struct ata_port *ap)
>
> /* Workaround for data length mismatch errata */
> if (unlikely(hstatus & INT_ON_DATA_LENGTH_MISMATCH)) {
> - for (tag = 0; tag < ATA_MAX_QUEUE; tag++) {
> + for (tag = 0; tag <= ATA_MAX_QUEUE; tag++) {
> qc = ata_qc_from_tag(ap, tag);
> if (qc && ata_is_atapi(qc->tf.protocol)) {
> u32 hcontrol;
>
> --
> Jens Axboe
^ permalink raw reply
* Re: [v3] powerpc/64s/radix: Fix MADV_[FREE|DONTNEED] TLB flush miss problem with THP
From: Michael Ellerman @ 2018-06-19 23:21 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Nicholas Piggin
In-Reply-To: <20180615013837.21710-1-npiggin@gmail.com>
On Fri, 2018-06-15 at 01:38:37 UTC, Nicholas Piggin wrote:
> The patch 99baac21e4 ("mm: fix MADV_[FREE|DONTNEED] TLB flush miss
> problem") added a force flush mode to the mmu_gather flush, which
> unconditionally flushes the entire address range being invalidated
> (even if actual ptes only covered a smaller range), to solve a problem
> with concurrent threads invalidating the same PTEs causing them to
> miss TLBs that need flushing.
>
> This does not work with powerpc that invalidates mmu_gather batches
> according to page size. Have powerpc flush all possible page sizes in
> the range if it encounters this concurrency condition.
>
> Patch 4647706ebe ("mm: always flush VMA ranges affected by
> zap_page_range") does add a TLB flush for all page sizes on powerpc for
> the zap_page_range case, but that is to be removed and replaced with
> the mmu_gather flush to avoid redundant flushing. It is also thought to
> not cover other obscure race conditions:
>
> https://lkml.kernel.org/r/BD3A0EBE-ECF4-41D4-87FA-C755EA9AB6BD@gmail.com
>
> Hash does not have a problem because it invalidates TLBs inside the
> page table locks.
>
> Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Applied to powerpc fixes, thanks.
https://git.kernel.org/powerpc/c/02390f66bd2362df114a0a0770d80e
cheers
^ permalink raw reply
* Re: powerpc/e500mc: Set assembler machine type to e500mc
From: Michael Ellerman @ 2018-06-19 23:21 UTC (permalink / raw)
To: Michael Jeanson
Cc: stable, Michael Jeanson, Paul Mackerras, Mathieu Desnoyers,
Vakul Garg, linuxppc-dev, Scott Wood, linux-kernel
In-Reply-To: <20180614152742.21075-1-mjeanson@efficios.com>
On Thu, 2018-06-14 at 15:27:42 UTC, Michael Jeanson wrote:
> In binutils 2.26 a new opcode for the "wait" instruction was added for the
> POWER9 and has precedence over the one specific to the e500mc. Commit
> ebf714ff3756 ("powerpc/e500mc: Add support for the wait instruction in
> e500_idle") uses this instruction specifically on the e500mc to work around
> an erratum.
>
> This results in an invalid instruction in idle_e500 when we build for the
> e500mc on bintutils >= 2.26 with the default assembler machine type.
>
> Since multiplatform between e500 and non-e500 is not supported, set the
> assembler machine type globaly when CONFIG_PPC_E500MC=y.
>
> Signed-off-by: Michael Jeanson <mjeanson@efficios.com>
> Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: Paul Mackerras <paulus@samba.org>
> CC: Michael Ellerman <mpe@ellerman.id.au>
> CC: Kumar Gala <galak@kernel.crashing.org>
> CC: Vakul Garg <vakul.garg@nxp.com>
> CC: Scott Wood <swood@redhat.com>
> CC: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> CC: linuxppc-dev@lists.ozlabs.org
> CC: linux-kernel@vger.kernel.org
> CC: stable@vger.kernel.org
Applied to powerpc fixes, thanks.
https://git.kernel.org/powerpc/c/69a8405999aa1c489de4b8d349468f
cheers
^ permalink raw reply
* Re: powerpc/64s: Fix DT CPU features Power9 DD2.1 logic
From: Michael Ellerman @ 2018-06-19 23:21 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: paulus, npiggin
In-Reply-To: <20180613132356.29246-1-mpe@ellerman.id.au>
On Wed, 2018-06-13 at 13:23:56 UTC, Michael Ellerman wrote:
> In the device tree CPU features quirk code we want to set
> CPU_FTR_POWER9_DD2_1 on all Power9s that aren't DD2.0 or earlier. But
> we got the logic wrong and instead set it on all CPUs that aren't
> Power9 DD2.0 or earlier, ie. including Power8.
>
> Fix it by making sure we're on a Power9. This isn't a bug in practice
> because the only code that checks the feature is Power9 only to begin
> with. But we'll backport it anyway to avoid confusion.
>
> Fixes: 9e9626ed3a4a ("powerpc/64s: Fix POWER9 DD2.2 and above in DT CPU features")
> Cc: stable@vger.kernel.org # v4.17+
> Reported-by: Paul Mackerras <paulus@samba.org>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Acked-by: Nicholas Piggin <npiggin@gmail.com>
Applied to powerpc fixes.
https://git.kernel.org/powerpc/c/749a0278c2177b2d16da5d8b135ba7
cheers
^ permalink raw reply
* Re: [1/3] powerpc/64: hard disable irqs in panic_smp_self_stop
From: Michael Ellerman @ 2018-06-19 23:21 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Nicholas Piggin
In-Reply-To: <20180519043554.26640-2-npiggin@gmail.com>
On Sat, 2018-05-19 at 04:35:52 UTC, Nicholas Piggin wrote:
> Similarly to commit 855bfe0de1 ("powerpc: hard disable irqs in
> smp_send_stop loop"), irqs should be hard disabled by
> panic_smp_self_stop.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Series applied to powerpc fixes, thanks.
https://git.kernel.org/powerpc/c/8c1aef6a682f87a059f10ab606cc1e
cheers
^ permalink raw reply
* [PATCH] powerpc/64s: Fix build failures with CONFIG_NMI_IPI=n
From: Michael Ellerman @ 2018-06-19 23:04 UTC (permalink / raw)
To: linuxppc-dev
I broke the build when CONFIG_NMI_IPI=n with my recent commit to add
arch_trigger_cpumask_backtrace(), eg:
stacktrace.c:(.text+0x1b0): undefined reference to `.smp_send_safe_nmi_ipi'
We should rework the CONFIG symbols here in future to avoid these
double barrelled ifdefs but for now they fix the build.
Fixes: 5cc05910f26e ("powerpc/64s: Wire up arch_trigger_cpumask_backtrace()")
Reported-by: Christophe LEROY <christophe.leroy@c-s.fr>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/include/asm/nmi.h | 2 +-
arch/powerpc/kernel/stacktrace.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/nmi.h b/arch/powerpc/include/asm/nmi.h
index 0f571e0ebca1..bd9ba8defd72 100644
--- a/arch/powerpc/include/asm/nmi.h
+++ b/arch/powerpc/include/asm/nmi.h
@@ -8,7 +8,7 @@ extern void arch_touch_nmi_watchdog(void);
static inline void arch_touch_nmi_watchdog(void) {}
#endif
-#if defined(CONFIG_PPC_BOOK3S_64) && defined(CONFIG_STACKTRACE)
+#if defined(CONFIG_NMI_IPI) && defined(CONFIG_STACKTRACE)
extern void arch_trigger_cpumask_backtrace(const cpumask_t *mask,
bool exclude_self);
#define arch_trigger_cpumask_backtrace arch_trigger_cpumask_backtrace
diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
index 07e97f289c52..e2c50b55138f 100644
--- a/arch/powerpc/kernel/stacktrace.c
+++ b/arch/powerpc/kernel/stacktrace.c
@@ -196,7 +196,7 @@ save_stack_trace_tsk_reliable(struct task_struct *tsk,
EXPORT_SYMBOL_GPL(save_stack_trace_tsk_reliable);
#endif /* CONFIG_HAVE_RELIABLE_STACKTRACE */
-#ifdef CONFIG_PPC_BOOK3S_64
+#if defined(CONFIG_PPC_BOOK3S_64) && defined(CONFIG_NMI_IPI)
static void handle_backtrace_ipi(struct pt_regs *regs)
{
nmi_cpu_backtrace(regs);
@@ -242,4 +242,4 @@ void arch_trigger_cpumask_backtrace(const cpumask_t *mask, bool exclude_self)
{
nmi_trigger_cpumask_backtrace(mask, exclude_self, raise_backtrace_ipi);
}
-#endif /* CONFIG_PPC64 */
+#endif /* defined(CONFIG_PPC_BOOK3S_64) && defined(CONFIG_NMI_IPI) */
--
2.14.1
^ permalink raw reply related
* Re: [git pull] Please pull powerpc.git next branch
From: Linus Torvalds @ 2018-06-19 22:58 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: linuxppc-dev list, Andrew Morton, Linux Kernel Mailing List
In-Reply-To: <1281327087.2168.67.camel@pasglop>
On Wed, Jun 20, 2018 at 5:23 AM Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc.git merge
I love the alleged line removal, but there's nothing in that 'merge'
branch. It points to an ancient powerpc commit from 2016.
And please, signed tags. I know you do them, because you used to send me them.
Linus
^ permalink raw reply
* Re: [PATCH] Revert "net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends"
From: Eric Dumazet @ 2018-06-19 22:40 UTC (permalink / raw)
To: Andreas Schwab, Eric Dumazet
Cc: Mathieu Malaterre, David S. Miller, Eric Dumazet, LKML,
Christophe LEROY, Meelis Roos, netdev, linuxppc-dev
In-Reply-To: <87lgbav24y.fsf@igel.home>
On 06/19/2018 03:32 PM, Andreas Schwab wrote:
> On Jun 19 2018, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>> diff --git a/drivers/net/ethernet/sun/sungem.c b/drivers/net/ethernet/sun/sungem.c
>> index 7a16d40a72d13cf1d522e8a3a396c826fe76f9b9..672d6748ab44f0890e92d5ca55d6ff6834c20dc9 100644
>> --- a/drivers/net/ethernet/sun/sungem.c
>> +++ b/drivers/net/ethernet/sun/sungem.c
>> @@ -60,8 +60,7 @@
>> #include <linux/sungem_phy.h>
>> #include "sungem.h"
>>
>> -/* Stripping FCS is causing problems, disabled for now */
>> -#undef STRIP_FCS
>> +#define STRIP_FCS
>>
>> #define DEFAULT_MSG (NETIF_MSG_DRV | \
>> NETIF_MSG_PROBE | \
>> @@ -435,7 +434,7 @@ static int gem_rxmac_reset(struct gem *gp)
>> writel(desc_dma & 0xffffffff, gp->regs + RXDMA_DBLOW);
>> writel(RX_RING_SIZE - 4, gp->regs + RXDMA_KICK);
>> val = (RXDMA_CFG_BASE | (RX_OFFSET << 10) |
>> - ((14 / 2) << 13) | RXDMA_CFG_FTHRESH_128);
>> + (ETH_HLEN << 13) | RXDMA_CFG_FTHRESH_128);
>> writel(val, gp->regs + RXDMA_CFG);
>> if (readl(gp->regs + GREG_BIFCFG) & GREG_BIFCFG_M66EN)
>> writel(((5 & RXDMA_BLANK_IPKTS) |
>> @@ -857,6 +856,14 @@ static int gem_rx(struct gem *gp, int work_to_do)
>>
>> csum = (__force __sum16)htons((status & RXDCTRL_TCPCSUM) ^ 0xffff);
>> skb->csum = csum_unfold(csum);
>> + {
>> + __wsum rsum = csum_partial(skb->data + ETH_HLEN, len - ETH_HLEN, 0);
>> + if (csum != csum_fold(rsum) && net_ratelimit())
>> + pr_err("sungem wrong csum : %x/%x, len %u bytes\n",
>> + csum, csum_fold(rsum), len);
>> + print_hex_dump(KERN_ERR, "raw data: ", DUMP_PREFIX_OFFSET,
>> + 16, 1, skb->data, len, true);
>> + }
>> skb->ip_summed = CHECKSUM_COMPLETE;
>> skb->protocol = eth_type_trans(skb, gp->dev);
>>
>> @@ -1761,7 +1768,7 @@ static void gem_init_dma(struct gem *gp)
>> writel(0, gp->regs + TXDMA_KICK);
>>
>> val = (RXDMA_CFG_BASE | (RX_OFFSET << 10) |
>> - ((14 / 2) << 13) | RXDMA_CFG_FTHRESH_128);
>> + (ETH_HLEN << 13) | RXDMA_CFG_FTHRESH_128);
>> writel(val, gp->regs + RXDMA_CFG);
>>
>> writel(desc_dma >> 32, gp->regs + RXDMA_DBHI);
>
> With that patch I still get the wrong csum messages, but no longer the
> hw csum failure messages (tested on a PowerMac G5).
>
> [ 662.659767] sungem: sungem wrong csum : 8359/7ca6, len 86 bytes, c0000001fee9cc02
> [ 662.659775] raw data: 00000000: 00 0d 93 43 81 62 d4 3d 7e 4c 48 b7 86 dd 61 01 ...C.b.=~LH...a.
> [ 662.659778] raw data: 00000010: 1c 1e 00 20 06 40 20 01 0a 62 17 11 88 01 00 00 ... .@ ..b......
> [ 662.659780] raw data: 00000020: 00 00 00 00 0a 38 20 01 0a 62 17 11 88 01 00 00 .....8 ..b......
> [ 662.659783] raw data: 00000030: 00 00 00 00 00 07 9a 18 00 16 c1 9a 7e ea ea 44 ............~..D
> [ 662.659785] raw data: 00000040: fb 4a 80 10 05 93 44 08 00 00 01 01 08 0a 59 68 .J....D.......Yh
> [ 662.659788] raw data: 00000050: ba e2 0e bb ac ae ......
>
> Andreas.
>
Note that 8359 and 7ca6 are the same really (a missing ~ to invert csum_partial())
So the bug was that :
1) Driver programmed a wrong start offset for the csum (7 bytes instead of 14 bytes to skip Ethernet Header)
2) FCS was not stripped.
Basically CHECKSUM_COMPLETE support never worked, but this was hidden by the fact that linux stack
had to throw away this CHECKSUM_COMPLETE because the FCS had to be removed.
Thanks !
^ permalink raw reply
* Re: [PATCH] Revert "net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends"
From: Andreas Schwab @ 2018-06-19 22:32 UTC (permalink / raw)
To: Eric Dumazet
Cc: Mathieu Malaterre, David S. Miller, Eric Dumazet, LKML,
Christophe LEROY, Meelis Roos, netdev, linuxppc-dev
In-Reply-To: <05645b90-d3bc-466d-116f-548f3ee39de9@gmail.com>
On Jun 19 2018, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> diff --git a/drivers/net/ethernet/sun/sungem.c b/drivers/net/ethernet/sun/sungem.c
> index 7a16d40a72d13cf1d522e8a3a396c826fe76f9b9..672d6748ab44f0890e92d5ca55d6ff6834c20dc9 100644
> --- a/drivers/net/ethernet/sun/sungem.c
> +++ b/drivers/net/ethernet/sun/sungem.c
> @@ -60,8 +60,7 @@
> #include <linux/sungem_phy.h>
> #include "sungem.h"
>
> -/* Stripping FCS is causing problems, disabled for now */
> -#undef STRIP_FCS
> +#define STRIP_FCS
>
> #define DEFAULT_MSG (NETIF_MSG_DRV | \
> NETIF_MSG_PROBE | \
> @@ -435,7 +434,7 @@ static int gem_rxmac_reset(struct gem *gp)
> writel(desc_dma & 0xffffffff, gp->regs + RXDMA_DBLOW);
> writel(RX_RING_SIZE - 4, gp->regs + RXDMA_KICK);
> val = (RXDMA_CFG_BASE | (RX_OFFSET << 10) |
> - ((14 / 2) << 13) | RXDMA_CFG_FTHRESH_128);
> + (ETH_HLEN << 13) | RXDMA_CFG_FTHRESH_128);
> writel(val, gp->regs + RXDMA_CFG);
> if (readl(gp->regs + GREG_BIFCFG) & GREG_BIFCFG_M66EN)
> writel(((5 & RXDMA_BLANK_IPKTS) |
> @@ -857,6 +856,14 @@ static int gem_rx(struct gem *gp, int work_to_do)
>
> csum = (__force __sum16)htons((status & RXDCTRL_TCPCSUM) ^ 0xffff);
> skb->csum = csum_unfold(csum);
> + {
> + __wsum rsum = csum_partial(skb->data + ETH_HLEN, len - ETH_HLEN, 0);
> + if (csum != csum_fold(rsum) && net_ratelimit())
> + pr_err("sungem wrong csum : %x/%x, len %u bytes\n",
> + csum, csum_fold(rsum), len);
> + print_hex_dump(KERN_ERR, "raw data: ", DUMP_PREFIX_OFFSET,
> + 16, 1, skb->data, len, true);
> + }
> skb->ip_summed = CHECKSUM_COMPLETE;
> skb->protocol = eth_type_trans(skb, gp->dev);
>
> @@ -1761,7 +1768,7 @@ static void gem_init_dma(struct gem *gp)
> writel(0, gp->regs + TXDMA_KICK);
>
> val = (RXDMA_CFG_BASE | (RX_OFFSET << 10) |
> - ((14 / 2) << 13) | RXDMA_CFG_FTHRESH_128);
> + (ETH_HLEN << 13) | RXDMA_CFG_FTHRESH_128);
> writel(val, gp->regs + RXDMA_CFG);
>
> writel(desc_dma >> 32, gp->regs + RXDMA_DBHI);
With that patch I still get the wrong csum messages, but no longer the
hw csum failure messages (tested on a PowerMac G5).
[ 662.659767] sungem: sungem wrong csum : 8359/7ca6, len 86 bytes, c0000001fee9cc02
[ 662.659775] raw data: 00000000: 00 0d 93 43 81 62 d4 3d 7e 4c 48 b7 86 dd 61 01 ...C.b.=~LH...a.
[ 662.659778] raw data: 00000010: 1c 1e 00 20 06 40 20 01 0a 62 17 11 88 01 00 00 ... .@ ..b......
[ 662.659780] raw data: 00000020: 00 00 00 00 0a 38 20 01 0a 62 17 11 88 01 00 00 .....8 ..b......
[ 662.659783] raw data: 00000030: 00 00 00 00 00 07 9a 18 00 16 c1 9a 7e ea ea 44 ............~..D
[ 662.659785] raw data: 00000040: fb 4a 80 10 05 93 44 08 00 00 01 01 08 0a 59 68 .J....D.......Yh
[ 662.659788] raw data: 00000050: ba e2 0e bb ac ae ......
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply
* Re: [PATCH] Revert "net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends"
From: Eric Dumazet @ 2018-06-19 22:10 UTC (permalink / raw)
To: Andreas Schwab
Cc: Mathieu Malaterre, David S. Miller, Eric Dumazet, LKML,
Christophe LEROY, Meelis Roos, netdev, linuxppc-dev
In-Reply-To: <e1790720-911b-4d04-2215-752ec612e3d7@gmail.com>
On 06/19/2018 01:10 PM, Eric Dumazet wrote:
>
>
> On 06/19/2018 12:10 PM, Andreas Schwab wrote:
>> On Jun 18 2018, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>>> DUMP_PREFIX_ADDRESS might give us more information (say alignment problem, or crossing page boundaries)
>>
>> DUMP_PREFIX_ADDRESS is useless for that purpose.
>>
>> Here are some samples of broken csums:
>>
>> [ 853.849225] sungem: sungem wrong csum : 9886/07be, len 94 bytes, c0000001fa187e02
>> [ 853.849232] raw data: 00000000: 00 0d 93 43 81 62 18 d6 c7 51 b8 1c 08 00 45 10 ...C.b...Q....E.
>> [ 853.849235] raw data: 00000010: 00 4c cb a0 40 00 40 11 d9 97 c0 a8 0a 01 c0 a8 .L..@.@.........
>> [ 853.849237] raw data: 00000020: 0a 07 00 7b 00 7b 00 38 69 e1 1c 03 0c f7 00 00 ...{.{.8i.......
>> [ 853.849240] raw data: 00000030: 08 f0 00 00 15 f0 c0 35 67 67 de d3 ca c9 d9 5b .......5gg.....[
>> [ 853.849242] raw data: 00000040: 1f ff de d3 d2 86 8f 67 fa f2 de d3 d2 86 8f 38 .......g.......8
>> [ 853.849244] raw data: 00000050: 2f ff de d3 d2 86 8f 3b ff ff d1 93 bc 50 /......;.....P
>
> Thanks.
>
> 4 bytes in excess.
>
> Might be the FCS, and it does not look like provided csum has a relation with it.
>
> For some reason FCS stripping was disabled by :
>
> commit 3e32011d4da6424b3bc65b1e1a047e30ac9882c7
> Author: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Date: Mon May 19 09:39:11 2003 -0700
>
> [SUNGEM]: Updates from PowerPC people.
>
> Support more chips and split out all the complex PHY
> handling into a seperate file.
>
>
> Since this NIC never had CHECKSUM_COMPLETE support (since we have to trim each skb,
> thus were forcing ip_summed to CHECKSUM_NONE) we probably should remove it and be happy.
>
> Unless you guys find a way to let the NIC strip the FCS, and double check the csum is a real csum ;)
>
> diff --git a/drivers/net/ethernet/sun/sungem.c b/drivers/net/ethernet/sun/sungem.c
> index 7a16d40a72d13cf1d522e8a3a396c826fe76f9b9..85439308375e95c3854e4a1561697d69ec85399b 100644
> --- a/drivers/net/ethernet/sun/sungem.c
> +++ b/drivers/net/ethernet/sun/sungem.c
> @@ -760,7 +760,6 @@ static int gem_rx(struct gem *gp, int work_to_do)
> struct net_device *dev = gp->dev;
> int entry, drops, work_done = 0;
> u32 done;
> - __sum16 csum;
>
> if (netif_msg_rx_status(gp))
> printk(KERN_DEBUG "%s: rx interrupt, done: %d, rx_new: %d\n",
> @@ -855,9 +854,6 @@ static int gem_rx(struct gem *gp, int work_to_do)
> skb = copy_skb;
> }
>
> - csum = (__force __sum16)htons((status & RXDCTRL_TCPCSUM) ^ 0xffff);
> - skb->csum = csum_unfold(csum);
> - skb->ip_summed = CHECKSUM_COMPLETE;
> skb->protocol = eth_type_trans(skb, gp->dev);
>
> napi_gro_receive(&gp->napi, skb);
>
If you have time, you also could check if changing the suspect (14 / 2) to ETH_HLEN would help
(and also enabling STRIP_FCS)
diff --git a/drivers/net/ethernet/sun/sungem.c b/drivers/net/ethernet/sun/sungem.c
index 7a16d40a72d13cf1d522e8a3a396c826fe76f9b9..672d6748ab44f0890e92d5ca55d6ff6834c20dc9 100644
--- a/drivers/net/ethernet/sun/sungem.c
+++ b/drivers/net/ethernet/sun/sungem.c
@@ -60,8 +60,7 @@
#include <linux/sungem_phy.h>
#include "sungem.h"
-/* Stripping FCS is causing problems, disabled for now */
-#undef STRIP_FCS
+#define STRIP_FCS
#define DEFAULT_MSG (NETIF_MSG_DRV | \
NETIF_MSG_PROBE | \
@@ -435,7 +434,7 @@ static int gem_rxmac_reset(struct gem *gp)
writel(desc_dma & 0xffffffff, gp->regs + RXDMA_DBLOW);
writel(RX_RING_SIZE - 4, gp->regs + RXDMA_KICK);
val = (RXDMA_CFG_BASE | (RX_OFFSET << 10) |
- ((14 / 2) << 13) | RXDMA_CFG_FTHRESH_128);
+ (ETH_HLEN << 13) | RXDMA_CFG_FTHRESH_128);
writel(val, gp->regs + RXDMA_CFG);
if (readl(gp->regs + GREG_BIFCFG) & GREG_BIFCFG_M66EN)
writel(((5 & RXDMA_BLANK_IPKTS) |
@@ -857,6 +856,14 @@ static int gem_rx(struct gem *gp, int work_to_do)
csum = (__force __sum16)htons((status & RXDCTRL_TCPCSUM) ^ 0xffff);
skb->csum = csum_unfold(csum);
+ {
+ __wsum rsum = csum_partial(skb->data + ETH_HLEN, len - ETH_HLEN, 0);
+ if (csum != csum_fold(rsum) && net_ratelimit())
+ pr_err("sungem wrong csum : %x/%x, len %u bytes\n",
+ csum, csum_fold(rsum), len);
+ print_hex_dump(KERN_ERR, "raw data: ", DUMP_PREFIX_OFFSET,
+ 16, 1, skb->data, len, true);
+ }
skb->ip_summed = CHECKSUM_COMPLETE;
skb->protocol = eth_type_trans(skb, gp->dev);
@@ -1761,7 +1768,7 @@ static void gem_init_dma(struct gem *gp)
writel(0, gp->regs + TXDMA_KICK);
val = (RXDMA_CFG_BASE | (RX_OFFSET << 10) |
- ((14 / 2) << 13) | RXDMA_CFG_FTHRESH_128);
+ (ETH_HLEN << 13) | RXDMA_CFG_FTHRESH_128);
writel(val, gp->regs + RXDMA_CFG);
writel(desc_dma >> 32, gp->regs + RXDMA_DBHI);
^ permalink raw reply related
* [PATCH] powerpc: enable kernel XZ compression option on BOOK3S_32
From: Aaro Koskinen @ 2018-06-19 20:52 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
linuxppc-dev
Cc: Aaro Koskinen
Enable kernel XZ compression option on BOOK3S_32. Tested on G4 PowerBook.
Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
---
arch/powerpc/platforms/Kconfig.cputype | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
index 67d3125d0610..aa8fe7fcc48e 100644
--- a/arch/powerpc/platforms/Kconfig.cputype
+++ b/arch/powerpc/platforms/Kconfig.cputype
@@ -25,6 +25,7 @@ choice
config PPC_BOOK3S_32
bool "512x/52xx/6xx/7xx/74xx/82xx/83xx/86xx"
select PPC_FPU
+ select HAVE_KERNEL_XZ
config PPC_85xx
bool "Freescale 85xx"
--
2.17.0
^ permalink raw reply related
* [PATCH v2 3/3] powerpc: Remove -Wattribute-alias pragmas
From: Paul Burton @ 2018-06-19 20:14 UTC (permalink / raw)
To: linux-kbuild
Cc: Mauro Carvalho Chehab, linux-mips, Arnd Bergmann, Ingo Molnar,
Matthew Wilcox, Thomas Gleixner, Douglas Anderson, Josh Poimboeuf,
Andrew Morton, Matthias Kaehlcke, He Zhe, Benjamin Herrenschmidt,
Michal Marek, Khem Raj, Christophe Leroy, Al Viro, Stafford Horne,
Gideon Israel Dsouza, Masahiro Yamada, Kees Cook,
Michael Ellerman, Heiko Carstens, linux-kernel, Paul Mackerras,
linuxppc-dev, Paul Burton
In-Reply-To: <20180619201458.4559-1-paul.burton@mips.com>
With SYSCALL_DEFINEx() disabling -Wattribute-alias generically, there's
no need to duplicate that for PowerPC syscalls.
This reverts commit 415520373975 ("powerpc: fix build failure by
disabling attribute-alias warning in pci_32") and commit 2479bfc9bc60
("powerpc: Fix build by disabling attribute-alias warning for
SYSCALL_DEFINEx").
Signed-off-by: Paul Burton <paul.burton@mips.com>
Cc: Michal Marek <michal.lkml@markovi.net>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Matthew Wilcox <matthew@wil.cx>
Cc: Matthias Kaehlcke <mka@chromium.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Gideon Israel Dsouza <gidisrael@gmail.com>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Stafford Horne <shorne@gmail.com>
Cc: Khem Raj <raj.khem@gmail.com>
Cc: He Zhe <zhe.he@windriver.com>
Cc: linux-kbuild@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-mips@linux-mips.org
Cc: linuxppc-dev@lists.ozlabs.org
---
Michael & Christophe, I didn't add your acks here yet since it changed
to include the second revert that Christophe pointed out I'd missed & I
didn't want to presume your acks extended to that.
Changes in v2:
- Also revert 2479bfc9bc60 ("powerpc: Fix build by disabling
attribute-alias warning for SYSCALL_DEFINEx").
- Change subject now that it's not just a simple one-commit revert.
arch/powerpc/kernel/pci_32.c | 4 ----
arch/powerpc/kernel/pci_64.c | 4 ----
arch/powerpc/kernel/rtas.c | 4 ----
arch/powerpc/kernel/signal_32.c | 8 --------
arch/powerpc/kernel/signal_64.c | 4 ----
arch/powerpc/kernel/syscalls.c | 4 ----
arch/powerpc/mm/subpage-prot.c | 4 ----
7 files changed, 32 deletions(-)
diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c
index 4f861055a852..d63b488d34d7 100644
--- a/arch/powerpc/kernel/pci_32.c
+++ b/arch/powerpc/kernel/pci_32.c
@@ -285,9 +285,6 @@ pci_bus_to_hose(int bus)
* Note that the returned IO or memory base is a physical address
*/
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wpragmas"
-#pragma GCC diagnostic ignored "-Wattribute-alias"
SYSCALL_DEFINE3(pciconfig_iobase, long, which,
unsigned long, bus, unsigned long, devfn)
{
@@ -313,4 +310,3 @@ SYSCALL_DEFINE3(pciconfig_iobase, long, which,
return result;
}
-#pragma GCC diagnostic pop
diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
index 812171c09f42..dff28f903512 100644
--- a/arch/powerpc/kernel/pci_64.c
+++ b/arch/powerpc/kernel/pci_64.c
@@ -203,9 +203,6 @@ void pcibios_setup_phb_io_space(struct pci_controller *hose)
#define IOBASE_ISA_IO 3
#define IOBASE_ISA_MEM 4
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wpragmas"
-#pragma GCC diagnostic ignored "-Wattribute-alias"
SYSCALL_DEFINE3(pciconfig_iobase, long, which, unsigned long, in_bus,
unsigned long, in_devfn)
{
@@ -259,7 +256,6 @@ SYSCALL_DEFINE3(pciconfig_iobase, long, which, unsigned long, in_bus,
return -EOPNOTSUPP;
}
-#pragma GCC diagnostic pop
#ifdef CONFIG_NUMA
int pcibus_to_node(struct pci_bus *bus)
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 7fb9f83dcde8..8afd146bc9c7 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -1051,9 +1051,6 @@ struct pseries_errorlog *get_pseries_errorlog(struct rtas_error_log *log,
}
/* We assume to be passed big endian arguments */
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wpragmas"
-#pragma GCC diagnostic ignored "-Wattribute-alias"
SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
{
struct rtas_args args;
@@ -1140,7 +1137,6 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
return 0;
}
-#pragma GCC diagnostic pop
/*
* Call early during boot, before mem init, to retrieve the RTAS
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 5eedbb282d42..e6474a45cef5 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -1038,9 +1038,6 @@ static int do_setcontext_tm(struct ucontext __user *ucp,
}
#endif
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wpragmas"
-#pragma GCC diagnostic ignored "-Wattribute-alias"
#ifdef CONFIG_PPC64
COMPAT_SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
struct ucontext __user *, new_ctx, int, ctx_size)
@@ -1134,7 +1131,6 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
set_thread_flag(TIF_RESTOREALL);
return 0;
}
-#pragma GCC diagnostic pop
#ifdef CONFIG_PPC64
COMPAT_SYSCALL_DEFINE0(rt_sigreturn)
@@ -1231,9 +1227,6 @@ SYSCALL_DEFINE0(rt_sigreturn)
return 0;
}
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wpragmas"
-#pragma GCC diagnostic ignored "-Wattribute-alias"
#ifdef CONFIG_PPC32
SYSCALL_DEFINE3(debug_setcontext, struct ucontext __user *, ctx,
int, ndbg, struct sig_dbg_op __user *, dbg)
@@ -1337,7 +1330,6 @@ SYSCALL_DEFINE3(debug_setcontext, struct ucontext __user *, ctx,
return 0;
}
#endif
-#pragma GCC diagnostic pop
/*
* OK, we're invoking a handler
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index d42b60020389..83d51bf586c7 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -625,9 +625,6 @@ static long setup_trampoline(unsigned int syscall, unsigned int __user *tramp)
/*
* Handle {get,set,swap}_context operations
*/
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wpragmas"
-#pragma GCC diagnostic ignored "-Wattribute-alias"
SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
struct ucontext __user *, new_ctx, long, ctx_size)
{
@@ -693,7 +690,6 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
set_thread_flag(TIF_RESTOREALL);
return 0;
}
-#pragma GCC diagnostic pop
/*
diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
index 083fa06962fd..466216506eb2 100644
--- a/arch/powerpc/kernel/syscalls.c
+++ b/arch/powerpc/kernel/syscalls.c
@@ -62,9 +62,6 @@ static inline long do_mmap2(unsigned long addr, size_t len,
return ret;
}
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wpragmas"
-#pragma GCC diagnostic ignored "-Wattribute-alias"
SYSCALL_DEFINE6(mmap2, unsigned long, addr, size_t, len,
unsigned long, prot, unsigned long, flags,
unsigned long, fd, unsigned long, pgoff)
@@ -78,7 +75,6 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, size_t, len,
{
return do_mmap2(addr, len, prot, flags, fd, offset, PAGE_SHIFT);
}
-#pragma GCC diagnostic pop
#ifdef CONFIG_PPC32
/*
diff --git a/arch/powerpc/mm/subpage-prot.c b/arch/powerpc/mm/subpage-prot.c
index 75cb646a79c3..9d16ee251fc0 100644
--- a/arch/powerpc/mm/subpage-prot.c
+++ b/arch/powerpc/mm/subpage-prot.c
@@ -186,9 +186,6 @@ static void subpage_mark_vma_nohuge(struct mm_struct *mm, unsigned long addr,
* in a 2-bit field won't allow writes to a page that is otherwise
* write-protected.
*/
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wpragmas"
-#pragma GCC diagnostic ignored "-Wattribute-alias"
SYSCALL_DEFINE3(subpage_prot, unsigned long, addr,
unsigned long, len, u32 __user *, map)
{
@@ -272,4 +269,3 @@ SYSCALL_DEFINE3(subpage_prot, unsigned long, addr,
up_write(&mm->mmap_sem);
return err;
}
-#pragma GCC diagnostic pop
--
2.17.1
^ permalink raw reply related
* [PATCH v2 2/3] disable -Wattribute-alias warning for SYSCALL_DEFINEx()
From: Paul Burton @ 2018-06-19 20:14 UTC (permalink / raw)
To: linux-kbuild
Cc: Mauro Carvalho Chehab, linux-mips, Arnd Bergmann, Ingo Molnar,
Matthew Wilcox, Thomas Gleixner, Douglas Anderson, Josh Poimboeuf,
Andrew Morton, Matthias Kaehlcke, He Zhe, Benjamin Herrenschmidt,
Michal Marek, Khem Raj, Christophe Leroy, Al Viro, Stafford Horne,
Gideon Israel Dsouza, Masahiro Yamada, Kees Cook,
Michael Ellerman, Heiko Carstens, linux-kernel, Paul Mackerras,
linuxppc-dev, Paul Burton
In-Reply-To: <20180619201458.4559-1-paul.burton@mips.com>
From: Arnd Bergmann <arnd@arndb.de>
gcc-8 warns for every single definition of a system call entry
point, e.g.:
include/linux/compat.h:56:18: error: 'compat_sys_rt_sigprocmask' alias between functions of incompatible types 'long int(int, compat_sigset_t *, compat_sigset_t *, compat_size_t)' {aka 'long int(int, struct <anonymous> *, struct <anonymous> *, unsigned int)'} and 'long int(long int, long int, long int, long int)' [-Werror=attribute-alias]
asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))\
^~~~~~~~~~
include/linux/compat.h:45:2: note: in expansion of macro 'COMPAT_SYSCALL_DEFINEx'
COMPAT_SYSCALL_DEFINEx(4, _##name, __VA_ARGS__)
^~~~~~~~~~~~~~~~~~~~~~
kernel/signal.c:2601:1: note: in expansion of macro 'COMPAT_SYSCALL_DEFINE4'
COMPAT_SYSCALL_DEFINE4(rt_sigprocmask, int, how, compat_sigset_t __user *, nset,
^~~~~~~~~~~~~~~~~~~~~~
include/linux/compat.h:60:18: note: aliased declaration here
asmlinkage long compat_SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__))\
^~~~~~~~~~
The new warning seems reasonable in principle, but it doesn't
help us here, since we rely on the type mismatch to sanitize the
system call arguments. After I reported this as GCC PR82435, a new
-Wno-attribute-alias option was added that could be used to turn the
warning off globally on the command line, but I'd prefer to do it a
little more fine-grained.
Interestingly, turning a warning off and on again inside of
a single macro doesn't always work, in this case I had to add
an extra statement inbetween and decided to copy the __SC_TEST
one from the native syscall to the compat syscall macro. See
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83256 for more details
about this.
[paul.burton@mips.com:
- Rebase atop current master.
- Split GCC & version arguments to __diag_ignore() in order to match
changes to the preceding patch.
- Add the comment argument to match the preceding patch.]
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82435
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Paul Burton <paul.burton@mips.com>
Tested-by: Christophe Leroy <christophe.leroy@c-s.fr>
Tested-by: Stafford Horne <shorne@gmail.com>
Cc: Michal Marek <michal.lkml@markovi.net>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Matthew Wilcox <matthew@wil.cx>
Cc: Matthias Kaehlcke <mka@chromium.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Gideon Israel Dsouza <gidisrael@gmail.com>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Stafford Horne <shorne@gmail.com>
Cc: Khem Raj <raj.khem@gmail.com>
Cc: He Zhe <zhe.he@windriver.com>
Cc: linux-kbuild@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-mips@linux-mips.org
Cc: linuxppc-dev@lists.ozlabs.org
---
Changes in v2: None
include/linux/compat.h | 8 +++++++-
include/linux/syscalls.h | 4 ++++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/include/linux/compat.h b/include/linux/compat.h
index b1a5562b3215..c68acc47da57 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -72,6 +72,9 @@
*/
#ifndef COMPAT_SYSCALL_DEFINEx
#define COMPAT_SYSCALL_DEFINEx(x, name, ...) \
+ __diag_push(); \
+ __diag_ignore(GCC, 8, "-Wattribute-alias", \
+ "Type aliasing is used to sanitize syscall arguments");\
asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)); \
asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
__attribute__((alias(__stringify(__se_compat_sys##name)))); \
@@ -80,8 +83,11 @@
asmlinkage long __se_compat_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)); \
asmlinkage long __se_compat_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \
{ \
- return __do_compat_sys##name(__MAP(x,__SC_DELOUSE,__VA_ARGS__));\
+ long ret = __do_compat_sys##name(__MAP(x,__SC_DELOUSE,__VA_ARGS__));\
+ __MAP(x,__SC_TEST,__VA_ARGS__); \
+ return ret; \
} \
+ __diag_pop(); \
static inline long __do_compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
#endif /* COMPAT_SYSCALL_DEFINEx */
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 73810808cdf2..a368a68cb667 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -231,6 +231,9 @@ static inline int is_syscall_trace_event(struct trace_event_call *tp_event)
*/
#ifndef __SYSCALL_DEFINEx
#define __SYSCALL_DEFINEx(x, name, ...) \
+ __diag_push(); \
+ __diag_ignore(GCC, 8, "-Wattribute-alias", \
+ "Type aliasing is used to sanitize syscall arguments");\
asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
__attribute__((alias(__stringify(__se_sys##name)))); \
ALLOW_ERROR_INJECTION(sys##name, ERRNO); \
@@ -243,6 +246,7 @@ static inline int is_syscall_trace_event(struct trace_event_call *tp_event)
__PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__)); \
return ret; \
} \
+ __diag_pop(); \
static inline long __do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
#endif /* __SYSCALL_DEFINEx */
--
2.17.1
^ permalink raw reply related
* [PATCH v2 1/3] kbuild: add macro for controlling warnings to linux/compiler.h
From: Paul Burton @ 2018-06-19 20:14 UTC (permalink / raw)
To: linux-kbuild
Cc: Mauro Carvalho Chehab, linux-mips, Arnd Bergmann, Ingo Molnar,
Matthew Wilcox, Thomas Gleixner, Douglas Anderson, Josh Poimboeuf,
Andrew Morton, Matthias Kaehlcke, He Zhe, Benjamin Herrenschmidt,
Michal Marek, Khem Raj, Christophe Leroy, Al Viro, Stafford Horne,
Gideon Israel Dsouza, Masahiro Yamada, Kees Cook,
Michael Ellerman, Heiko Carstens, linux-kernel, Paul Mackerras,
linuxppc-dev, Paul Burton
In-Reply-To: <20180619201458.4559-1-paul.burton@mips.com>
From: Arnd Bergmann <arnd@arndb.de>
I have occasionally run into a situation where it would make sense to
control a compiler warning from a source file rather than doing so from
a Makefile using the $(cc-disable-warning, ...) or $(cc-option, ...)
helpers.
The approach here is similar to what glibc uses, using __diag() and
related macros to encapsulate a _Pragma("GCC diagnostic ...") statement
that gets turned into the respective "#pragma GCC diagnostic ..." by
the preprocessor when the macro gets expanded.
Like glibc, I also have an argument to pass the affected compiler
version, but decided to actually evaluate that one. For now, this
supports GCC_4_6, GCC_4_7, GCC_4_8, GCC_4_9, GCC_5, GCC_6, GCC_7,
GCC_8 and GCC_9. Adding support for CLANG_5 and other interesting
versions is straightforward here. GNU compilers starting with gcc-4.2
could support it in principle, but "#pragma GCC diagnostic push"
was only added in gcc-4.6, so it seems simpler to not deal with those
at all. The same versions show a large number of warnings already,
so it seems easier to just leave it at that and not do a more
fine-grained control for them.
The use cases I found so far include:
- turning off the gcc-8 -Wattribute-alias warning inside of the
SYSCALL_DEFINEx() macro without having to do it globally.
- Reducing the build time for a simple re-make after a change,
once we move the warnings from ./Makefile and
./scripts/Makefile.extrawarn into linux/compiler.h
- More control over the warnings based on other configurations,
using preprocessor syntax instead of Makefile syntax. This should make
it easier for the average developer to understand and change things.
- Adding an easy way to turn the W=1 option on unconditionally
for a subdirectory or a specific file. This has been requested
by several developers in the past that want to have their subsystems
W=1 clean.
- Integrating clang better into the build systems. Clang supports
more warnings than GCC, and we probably want to classify them
as default, W=1, W=2 etc, but there are cases in which the
warnings should be classified differently due to excessive false
positives from one or the other compiler.
- Adding a way to turn the default warnings into errors (e.g. using
a new "make E=0" tag) while not also turning the W=1 warnings into
errors.
This patch for now just adds the minimal infrastructure in order to
do the first of the list above. As the #pragma GCC diagnostic
takes precedence over command line options, the next step would be
to convert a lot of the individual Makefiles that set nonstandard
options to use __diag() instead.
[paul.burton@mips.com:
- Rebase atop current master.
- Add __diag_GCC, or more generally __diag_<compiler>, abstraction to
avoid code outside of linux/compiler-gcc.h needing to duplicate
knowledge about different GCC versions.
- Add a comment argument to __diag_{ignore,warn,error} which isn't
used in the expansion of the macros but serves to push people to
document the reason for using them - per feedback from Kees Cook.
- Translate severity to GCC-specific pragmas in linux/compiler-gcc.h
rather than using GCC-specific in linux/compiler_types.h.
- Drop all but GCC 8 macros, since we only need to define macros for
versions that we need to introduce pragmas for, and as of this
series that's just GCC 8.
- Capitalize comments in linux/compiler-gcc.h to match the style of
the rest of the file.
- Line up macro definitions with tabs in linux/compiler-gcc.h.]
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Paul Burton <paul.burton@mips.com>
Tested-by: Christophe Leroy <christophe.leroy@c-s.fr>
Tested-by: Stafford Horne <shorne@gmail.com>
Cc: Michal Marek <michal.lkml@markovi.net>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Matthew Wilcox <matthew@wil.cx>
Cc: Matthias Kaehlcke <mka@chromium.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Gideon Israel Dsouza <gidisrael@gmail.com>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Stafford Horne <shorne@gmail.com>
Cc: Khem Raj <raj.khem@gmail.com>
Cc: He Zhe <zhe.he@windriver.com>
Cc: linux-kbuild@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-mips@linux-mips.org
Cc: linuxppc-dev@lists.ozlabs.org
---
Changes in v2:
- Add version argument to fallback __diag_GCC definition.
- Translate severity from generic ignore,warn,error to GCC-specific
pragma content ignored,warning,error in linux/compiler-gcc.h in order
to keep linux/compiler_types.h generic per feedback from Masahiro
Yamada.
- Drop all but GCC 8 macros, since we only need to define macros for
versions that we need to introduce pragmas for, and as of this series
that's just GCC 8.
- Capitalize comments in linux/compiler-gcc.h to match the style of the
rest of the file.
- Line up macro definitions with tabs in linux/compiler-gcc.h.
include/linux/compiler-gcc.h | 27 +++++++++++++++++++++++++++
include/linux/compiler_types.h | 18 ++++++++++++++++++
2 files changed, 45 insertions(+)
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index f1a7492a5cc8..5067a90af9c3 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -347,3 +347,30 @@
#if GCC_VERSION >= 50100
#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
#endif
+
+/*
+ * Turn individual warnings and errors on and off locally, depending
+ * on version.
+ */
+#define __diag_GCC(version, severity, s) \
+ __diag_GCC_ ## version(__diag_GCC_ ## severity s)
+
+/* Severity used in pragma directives */
+#define __diag_GCC_ignore ignored
+#define __diag_GCC_warn warning
+#define __diag_GCC_error error
+
+/* Compilers before gcc-4.6 do not understand "#pragma GCC diagnostic push" */
+#if GCC_VERSION >= 40600
+#define __diag_str1(s) #s
+#define __diag_str(s) __diag_str1(s)
+#define __diag(s) _Pragma(__diag_str(GCC diagnostic s))
+#else
+#define __diag(s)
+#endif
+
+#if GCC_VERSION >= 80000
+#define __diag_GCC_8(s) __diag(s)
+#else
+#define __diag_GCC_8(s)
+#endif
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 6b79a9bba9a7..a8ba6b04152c 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -271,4 +271,22 @@ struct ftrace_likely_data {
# define __native_word(t) (sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long))
#endif
+#ifndef __diag
+#define __diag(string)
+#endif
+
+#ifndef __diag_GCC
+#define __diag_GCC(version, severity, string)
+#endif
+
+#define __diag_push() __diag(push)
+#define __diag_pop() __diag(pop)
+
+#define __diag_ignore(compiler, version, option, comment) \
+ __diag_ ## compiler(version, ignore, option)
+#define __diag_warn(compiler, version, option, comment) \
+ __diag_ ## compiler(version, warn, option)
+#define __diag_error(compiler, version, option, comment) \
+ __diag_ ## compiler(version, error, option)
+
#endif /* __LINUX_COMPILER_TYPES_H */
--
2.17.1
^ permalink raw reply related
* [PATCH v2 0/3] Resolve -Wattribute-alias warnings from SYSCALL_DEFINEx()
From: Paul Burton @ 2018-06-19 20:14 UTC (permalink / raw)
To: linux-kbuild
Cc: Mauro Carvalho Chehab, linux-mips, Arnd Bergmann, Ingo Molnar,
Matthew Wilcox, Thomas Gleixner, Douglas Anderson, Josh Poimboeuf,
Andrew Morton, Matthias Kaehlcke, He Zhe, Benjamin Herrenschmidt,
Michal Marek, Khem Raj, Christophe Leroy, Al Viro, Stafford Horne,
Gideon Israel Dsouza, Masahiro Yamada, Kees Cook,
Michael Ellerman, Heiko Carstens, linux-kernel, Paul Mackerras,
linuxppc-dev, Paul Burton
In-Reply-To: <20180619190225.7eguhiw3ixaiwpgl@pburton-laptop>
This series introduces infrastructure allowing compiler diagnostics to
be disabled or their severity modified for specific pieces of code, with
suitable abstractions to prevent that code from becoming tied to a
specific compiler.
This infrastructure is then used to disable the -Wattribute-alias
warning around syscall definitions, which rely on type mismatches to
sanitize arguments.
Finally PowerPC-specific #pragma's are removed now that the generic code
is handling this.
The series takes Arnd's RFC patches & addresses the review comments they
received. The most notable effect of this series to to avoid warnings &
build failures caused by -Wattribute-alias when compiling the kernel
with GCC 8.
Applies cleanly atop v4.18-rc1.
Thanks,
Paul
Arnd Bergmann (2):
kbuild: add macro for controlling warnings to linux/compiler.h
disable -Wattribute-alias warning for SYSCALL_DEFINEx()
Paul Burton (1):
powerpc: Remove -Wattribute-alias pragmas
arch/powerpc/kernel/pci_32.c | 4 ----
arch/powerpc/kernel/pci_64.c | 4 ----
arch/powerpc/kernel/rtas.c | 4 ----
arch/powerpc/kernel/signal_32.c | 8 --------
arch/powerpc/kernel/signal_64.c | 4 ----
arch/powerpc/kernel/syscalls.c | 4 ----
arch/powerpc/mm/subpage-prot.c | 4 ----
include/linux/compat.h | 8 +++++++-
include/linux/compiler-gcc.h | 27 +++++++++++++++++++++++++++
include/linux/compiler_types.h | 18 ++++++++++++++++++
include/linux/syscalls.h | 4 ++++
11 files changed, 56 insertions(+), 33 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: [PATCH] Revert "net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends"
From: Eric Dumazet @ 2018-06-19 20:10 UTC (permalink / raw)
To: Andreas Schwab, Eric Dumazet
Cc: Mathieu Malaterre, David S. Miller, Eric Dumazet, LKML,
Christophe LEROY, Meelis Roos, netdev, linuxppc-dev
In-Reply-To: <87po0mvbgl.fsf@igel.home>
On 06/19/2018 12:10 PM, Andreas Schwab wrote:
> On Jun 18 2018, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>> DUMP_PREFIX_ADDRESS might give us more information (say alignment problem, or crossing page boundaries)
>
> DUMP_PREFIX_ADDRESS is useless for that purpose.
>
> Here are some samples of broken csums:
>
> [ 853.849225] sungem: sungem wrong csum : 9886/07be, len 94 bytes, c0000001fa187e02
> [ 853.849232] raw data: 00000000: 00 0d 93 43 81 62 18 d6 c7 51 b8 1c 08 00 45 10 ...C.b...Q....E.
> [ 853.849235] raw data: 00000010: 00 4c cb a0 40 00 40 11 d9 97 c0 a8 0a 01 c0 a8 .L..@.@.........
> [ 853.849237] raw data: 00000020: 0a 07 00 7b 00 7b 00 38 69 e1 1c 03 0c f7 00 00 ...{.{.8i.......
> [ 853.849240] raw data: 00000030: 08 f0 00 00 15 f0 c0 35 67 67 de d3 ca c9 d9 5b .......5gg.....[
> [ 853.849242] raw data: 00000040: 1f ff de d3 d2 86 8f 67 fa f2 de d3 d2 86 8f 38 .......g.......8
> [ 853.849244] raw data: 00000050: 2f ff de d3 d2 86 8f 3b ff ff d1 93 bc 50 /......;.....P
Thanks.
4 bytes in excess.
Might be the FCS, and it does not look like provided csum has a relation with it.
For some reason FCS stripping was disabled by :
commit 3e32011d4da6424b3bc65b1e1a047e30ac9882c7
Author: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Mon May 19 09:39:11 2003 -0700
[SUNGEM]: Updates from PowerPC people.
Support more chips and split out all the complex PHY
handling into a seperate file.
Since this NIC never had CHECKSUM_COMPLETE support (since we have to trim each skb,
thus were forcing ip_summed to CHECKSUM_NONE) we probably should remove it and be happy.
Unless you guys find a way to let the NIC strip the FCS, and double check the csum is a real csum ;)
diff --git a/drivers/net/ethernet/sun/sungem.c b/drivers/net/ethernet/sun/sungem.c
index 7a16d40a72d13cf1d522e8a3a396c826fe76f9b9..85439308375e95c3854e4a1561697d69ec85399b 100644
--- a/drivers/net/ethernet/sun/sungem.c
+++ b/drivers/net/ethernet/sun/sungem.c
@@ -760,7 +760,6 @@ static int gem_rx(struct gem *gp, int work_to_do)
struct net_device *dev = gp->dev;
int entry, drops, work_done = 0;
u32 done;
- __sum16 csum;
if (netif_msg_rx_status(gp))
printk(KERN_DEBUG "%s: rx interrupt, done: %d, rx_new: %d\n",
@@ -855,9 +854,6 @@ static int gem_rx(struct gem *gp, int work_to_do)
skb = copy_skb;
}
- csum = (__force __sum16)htons((status & RXDCTRL_TCPCSUM) ^ 0xffff);
- skb->csum = csum_unfold(csum);
- skb->ip_summed = CHECKSUM_COMPLETE;
skb->protocol = eth_type_trans(skb, gp->dev);
napi_gro_receive(&gp->napi, skb);
^ permalink raw reply related
* Re: [PATCH v1] mm: relax deferred struct page requirements
From: Pavel Tatashin @ 2018-06-19 19:56 UTC (permalink / raw)
To: jslaby
Cc: mhocko, Steven Sistare, Daniel Jordan, benh, paulus,
Andrew Morton, kirill.shutemov, Reza Arbab, schwidefsky,
Heiko Carstens, x86, LKML, tglx, linuxppc-dev,
Linux Memory Management List, linux-s390, mgorman
In-Reply-To: <CAGM2rea=_VJJ26tohWQWgfwcFVkp0gb6j1edH1kVLjtxfugf5Q@mail.gmail.com>
On Tue, Jun 19, 2018 at 9:50 AM Pavel Tatashin
<pasha.tatashin@oracle.com> wrote:
>
> On Sat, Jun 16, 2018 at 4:04 AM Jiri Slaby <jslaby@suse.cz> wrote:
> >
> > On 11/21/2017, 08:24 AM, Michal Hocko wrote:
> > > On Thu 16-11-17 20:46:01, Pavel Tatashin wrote:
> > >> There is no need to have ARCH_SUPPORTS_DEFERRED_STRUCT_PAGE_INIT,
> > >> as all the page initialization code is in common code.
> > >>
> > >> Also, there is no need to depend on MEMORY_HOTPLUG, as initialization code
> > >> does not really use hotplug memory functionality. So, we can remove this
> > >> requirement as well.
> > >>
> > >> This patch allows to use deferred struct page initialization on all
> > >> platforms with memblock allocator.
> > >>
> > >> Tested on x86, arm64, and sparc. Also, verified that code compiles on
> > >> PPC with CONFIG_MEMORY_HOTPLUG disabled.
> > >
> > > There is slight risk that we will encounter corner cases on some
> > > architectures with weird memory layout/topology
> >
> > Which x86_32-pae seems to be. Many bad page state errors are emitted
> > during boot when this patch is applied:
>
> Hi Jiri,
>
> Thank you for reporting this bug.
>
> Because 32-bit systems are limited in the maximum amount of physical
> memory, they don't need deferred struct pages. So, we can add depends
> on 64BIT to DEFERRED_STRUCT_PAGE_INIT in mm/Kconfig.
>
> However, before we do this, I want to try reproducing this problem and
> root cause it, as it might expose a general problem that is not 32-bit
> specific.
Hi Jiri,
Could you please attach your config and full qemu arguments that you
used to reproduce this bug.
Thank you,
Pavel
>
> Thank you,
> Pavel
^ permalink raw reply
* [PATCH 1/2] powerpc: Flush checkpointed gpr state for 32-bit processes in ptrace
From: Pedro Franco de Carvalho @ 2018-06-19 19:54 UTC (permalink / raw)
To: linuxppc-dev, mpe
In-Reply-To: <87d0wvtolp.fsf@concordia.ellerman.id.au>
Would something like this be ok?
I factored out the calls to flush_fp_to_thread and flush_altivec_to_thread,
although I don't really understand why these are necessary. The tm_cvsx_get/set
functions also calls flush_vsx_to_thread (outside of the helper function).
I also noticed that tm_ppr/dscr/tar_get/set functions don't flush the tm
state. Should they do it, and if so, should they also flush the fp and altivec
state?
Thanks!
Pedro
-- >8 --
Currently ptrace doesn't flush the register state when the
checkpointed GPRs of a 32-bit thread are accessed. This can cause core
dumps to have stale data in the checkpointed GPR note.
This patch adds a helper function to flush the TM, fpu and altivec
state and calls it from the tm_cgpr32_get/set functions.
Signed-off-by: Pedro Franco de Carvalho <pedromfc@linux.vnet.ibm.com>
---
arch/powerpc/kernel/ptrace.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 9667666eb18e..0d56857e1e89 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -778,6 +778,29 @@ static int evr_set(struct task_struct *target, const struct user_regset *regset,
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
/**
+ * tm_flush_if_active - flush TM, fpu and altivec state if TM active
+ * @target: The target task.
+ *
+ * This function flushes the TM, fpu and altivec state to the target
+ * task and returns 0 if TM is available and active in the target, and
+ * returns an error code suitable for ptrace otherwise.
+ */
+static int tm_flush_if_active (struct task_struct *target)
+{
+ if (!cpu_has_feature(CPU_FTR_TM))
+ return -ENODEV;
+
+ if (!MSR_TM_ACTIVE(target->thread.regs->msr))
+ return -ENODATA;
+
+ flush_tmregs_to_thread(target);
+ flush_fp_to_thread(target);
+ flush_altivec_to_thread(target);
+
+ return 0;
+}
+
+/**
* tm_cgpr_active - get active number of registers in CGPR
* @target: The target task.
* @regset: The user regset structure.
@@ -2124,6 +2147,11 @@ static int tm_cgpr32_get(struct task_struct *target,
unsigned int pos, unsigned int count,
void *kbuf, void __user *ubuf)
{
+ int ret = tm_flush_if_active(target);
+
+ if (ret)
+ return ret;
+
return gpr32_get_common(target, regset, pos, count, kbuf, ubuf,
&target->thread.ckpt_regs.gpr[0]);
}
@@ -2133,6 +2161,11 @@ static int tm_cgpr32_set(struct task_struct *target,
unsigned int pos, unsigned int count,
const void *kbuf, const void __user *ubuf)
{
+ int ret = tm_flush_if_active(target);
+
+ if (ret)
+ return ret;
+
return gpr32_set_common(target, regset, pos, count, kbuf, ubuf,
&target->thread.ckpt_regs.gpr[0]);
}
--
2.13.6
^ permalink raw reply related
* [PATCH 2/2] powerpc: Use helper function to flush TM state in ptrace
From: Pedro Franco de Carvalho @ 2018-06-19 19:54 UTC (permalink / raw)
To: linuxppc-dev, mpe
In-Reply-To: <87d0wvtolp.fsf@concordia.ellerman.id.au>
This patch updates the get/set regset functions that flush tm, fpu and
altvec state when TM is available and active to use a helper function.
Signed-off-by: Pedro Franco de Carvalho <pedromfc@linux.vnet.ibm.com>
---
arch/powerpc/kernel/ptrace.c | 104 ++++++++++++-------------------------------
1 file changed, 28 insertions(+), 76 deletions(-)
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 0d56857e1e89..84fa97587850 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -845,17 +845,10 @@ static int tm_cgpr_get(struct task_struct *target,
unsigned int pos, unsigned int count,
void *kbuf, void __user *ubuf)
{
- int ret;
-
- if (!cpu_has_feature(CPU_FTR_TM))
- return -ENODEV;
-
- if (!MSR_TM_ACTIVE(target->thread.regs->msr))
- return -ENODATA;
+ int ret = tm_flush_if_active(target);
- flush_tmregs_to_thread(target);
- flush_fp_to_thread(target);
- flush_altivec_to_thread(target);
+ if (ret)
+ return ret;
ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
&target->thread.ckpt_regs,
@@ -910,17 +903,11 @@ static int tm_cgpr_set(struct task_struct *target,
const void *kbuf, const void __user *ubuf)
{
unsigned long reg;
- int ret;
-
- if (!cpu_has_feature(CPU_FTR_TM))
- return -ENODEV;
- if (!MSR_TM_ACTIVE(target->thread.regs->msr))
- return -ENODATA;
+ int ret = tm_flush_if_active(target);
- flush_tmregs_to_thread(target);
- flush_fp_to_thread(target);
- flush_altivec_to_thread(target);
+ if (ret)
+ return ret;
ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
&target->thread.ckpt_regs,
@@ -1014,15 +1001,10 @@ static int tm_cfpr_get(struct task_struct *target,
u64 buf[33];
int i;
- if (!cpu_has_feature(CPU_FTR_TM))
- return -ENODEV;
-
- if (!MSR_TM_ACTIVE(target->thread.regs->msr))
- return -ENODATA;
+ int ret = tm_flush_if_active(target);
- flush_tmregs_to_thread(target);
- flush_fp_to_thread(target);
- flush_altivec_to_thread(target);
+ if (ret)
+ return ret;
/* copy to local buffer then write that out */
for (i = 0; i < 32 ; i++)
@@ -1060,15 +1042,10 @@ static int tm_cfpr_set(struct task_struct *target,
u64 buf[33];
int i;
- if (!cpu_has_feature(CPU_FTR_TM))
- return -ENODEV;
-
- if (!MSR_TM_ACTIVE(target->thread.regs->msr))
- return -ENODATA;
+ int ret = tm_flush_if_active(target);
- flush_tmregs_to_thread(target);
- flush_fp_to_thread(target);
- flush_altivec_to_thread(target);
+ if (ret)
+ return ret;
for (i = 0; i < 32; i++)
buf[i] = target->thread.TS_CKFPR(i);
@@ -1131,20 +1108,12 @@ static int tm_cvmx_get(struct task_struct *target,
unsigned int pos, unsigned int count,
void *kbuf, void __user *ubuf)
{
- int ret;
-
- BUILD_BUG_ON(TVSO(vscr) != TVSO(vr[32]));
-
- if (!cpu_has_feature(CPU_FTR_TM))
- return -ENODEV;
+ int ret = tm_flush_if_active(target);
- if (!MSR_TM_ACTIVE(target->thread.regs->msr))
- return -ENODATA;
+ if (ret)
+ return ret;
- /* Flush the state */
- flush_tmregs_to_thread(target);
- flush_fp_to_thread(target);
- flush_altivec_to_thread(target);
+ BUILD_BUG_ON(TVSO(vscr) != TVSO(vr[32]));
ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
&target->thread.ckvr_state, 0,
@@ -1193,19 +1162,12 @@ static int tm_cvmx_set(struct task_struct *target,
unsigned int pos, unsigned int count,
const void *kbuf, const void __user *ubuf)
{
- int ret;
-
- BUILD_BUG_ON(TVSO(vscr) != TVSO(vr[32]));
-
- if (!cpu_has_feature(CPU_FTR_TM))
- return -ENODEV;
+ int ret = tm_flush_if_active(target);
- if (!MSR_TM_ACTIVE(target->thread.regs->msr))
- return -ENODATA;
+ if (ret)
+ return ret;
- flush_tmregs_to_thread(target);
- flush_fp_to_thread(target);
- flush_altivec_to_thread(target);
+ BUILD_BUG_ON(TVSO(vscr) != TVSO(vr[32]));
ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
&target->thread.ckvr_state, 0,
@@ -1276,18 +1238,13 @@ static int tm_cvsx_get(struct task_struct *target,
void *kbuf, void __user *ubuf)
{
u64 buf[32];
- int ret, i;
+ int i;
- if (!cpu_has_feature(CPU_FTR_TM))
- return -ENODEV;
+ int ret = tm_flush_if_active(target);
- if (!MSR_TM_ACTIVE(target->thread.regs->msr))
- return -ENODATA;
+ if (ret)
+ return ret;
- /* Flush the state */
- flush_tmregs_to_thread(target);
- flush_fp_to_thread(target);
- flush_altivec_to_thread(target);
flush_vsx_to_thread(target);
for (i = 0; i < 32 ; i++)
@@ -1324,18 +1281,13 @@ static int tm_cvsx_set(struct task_struct *target,
const void *kbuf, const void __user *ubuf)
{
u64 buf[32];
- int ret, i;
+ int i;
- if (!cpu_has_feature(CPU_FTR_TM))
- return -ENODEV;
+ int ret = tm_flush_if_active(target);
- if (!MSR_TM_ACTIVE(target->thread.regs->msr))
- return -ENODATA;
+ if (ret)
+ return ret;
- /* Flush the state */
- flush_tmregs_to_thread(target);
- flush_fp_to_thread(target);
- flush_altivec_to_thread(target);
flush_vsx_to_thread(target);
for (i = 0; i < 32 ; i++)
--
2.13.6
^ permalink raw reply related
* Re: [PATCH] Revert "net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends"
From: Andreas Schwab @ 2018-06-19 19:10 UTC (permalink / raw)
To: Eric Dumazet
Cc: Mathieu Malaterre, David S. Miller, Eric Dumazet, LKML,
Christophe LEROY, Meelis Roos, netdev, linuxppc-dev
In-Reply-To: <dd6f13bc-c5f2-85f8-c08d-837bc024fc7c@gmail.com>
On Jun 18 2018, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> DUMP_PREFIX_ADDRESS might give us more information (say alignment problem, or crossing page boundaries)
DUMP_PREFIX_ADDRESS is useless for that purpose.
Here are some samples of broken csums:
[ 853.849225] sungem: sungem wrong csum : 9886/07be, len 94 bytes, c0000001fa187e02
[ 853.849232] raw data: 00000000: 00 0d 93 43 81 62 18 d6 c7 51 b8 1c 08 00 45 10 ...C.b...Q....E.
[ 853.849235] raw data: 00000010: 00 4c cb a0 40 00 40 11 d9 97 c0 a8 0a 01 c0 a8 .L..@.@.........
[ 853.849237] raw data: 00000020: 0a 07 00 7b 00 7b 00 38 69 e1 1c 03 0c f7 00 00 ...{.{.8i.......
[ 853.849240] raw data: 00000030: 08 f0 00 00 15 f0 c0 35 67 67 de d3 ca c9 d9 5b .......5gg.....[
[ 853.849242] raw data: 00000040: 1f ff de d3 d2 86 8f 67 fa f2 de d3 d2 86 8f 38 .......g.......8
[ 853.849244] raw data: 00000050: 2f ff de d3 d2 86 8f 3b ff ff d1 93 bc 50 /......;.....P
[ 857.883052] sungem: sungem wrong csum : dbb4/c48f, len 94 bytes, c0000001fa185882
[ 857.883058] raw data: 00000000: 00 0d 93 43 81 62 18 d6 c7 51 b8 1c 08 00 45 00 ...C.b...Q....E.
[ 857.883070] raw data: 00000010: 00 4c a1 97 40 00 3a 11 ce ed d9 5b 2c 11 c0 a8 .L..@.:....[,...
[ 857.883080] raw data: 00000020: 0a 07 00 7b 00 7b 00 38 14 4b 24 02 06 ea 00 00 ...{.{.8.K$.....
[ 857.883085] raw data: 00000030: 00 0b 00 00 02 99 c0 a8 64 09 de d3 d2 5a 36 e4 ........d....Z6.
[ 857.883090] raw data: 00000040: bc f5 de d3 d2 8a 8f 2c 17 44 de d3 d2 8a 93 8b .......,.D......
[ 857.883094] raw data: 00000050: d7 b7 de d3 d2 8a 93 97 69 6e 39 7b d2 5a ........in9{.Z
[ 858.124689] sungem: sungem wrong csum : 1f4f/02d0, len 118 bytes, c0000001fa185602
[ 858.124700] raw data: 00000000: 00 0d 93 43 81 62 d4 3d 7e 4c 48 b7 86 dd 61 01 ...C.b.=~LH...a.
[ 858.124705] raw data: 00000010: 1e b1 00 3c 06 40 20 01 0a 62 17 11 88 01 00 00 ...<.@ ..b......
[ 858.124709] raw data: 00000020: 00 00 00 00 0a 38 20 01 0a 62 17 11 88 01 00 00 .....8 ..b......
[ 858.124714] raw data: 00000030: 00 00 00 00 00 07 94 b4 00 16 86 f5 29 e8 36 cb ............).6.
[ 858.124718] raw data: 00000040: 50 49 80 18 05 93 9a 53 00 00 01 01 08 0a 58 b2 PI.....S......X.
[ 858.124723] raw data: 00000050: de 54 61 5f 2f 3c 00 00 00 10 cc 08 55 f7 da 21 .Ta_/<......U..!
[ 858.124727] raw data: 00000060: f4 60 0a 6b 3c aa b9 b3 7e 61 10 b8 c2 be 9a 0b .`.k<...~a......
[ 858.124731] raw data: 00000070: c7 e9 5b 97 1b ac ..[...
[ 858.126522] sungem: sungem wrong csum : 0836/19e9, len 90 bytes, c0000001fa185382
[ 858.126530] raw data: 00000000: 00 0d 93 43 81 62 d4 3d 7e 4c 48 b7 86 dd 61 01 ...C.b.=~LH...a.
[ 858.126532] raw data: 00000010: 1e b1 00 20 06 40 20 01 0a 62 17 11 88 01 00 00 ... .@ ..b......
[ 858.126535] raw data: 00000020: 00 00 00 00 0a 38 20 01 0a 62 17 11 88 01 00 00 .....8 ..b......
[ 858.126537] raw data: 00000030: 00 00 00 00 00 07 94 b4 00 16 86 f5 2a 04 36 cb ............*.6.
[ 858.126540] raw data: 00000040: 50 65 80 10 05 93 3e 56 00 00 01 01 08 0a 58 b2 Pe....>V......X.
[ 858.126542] raw data: 00000050: de 56 61 5f 30 4d 1d 58 42 d2 .Va_0M.XB.
[ 858.131559] sungem: sungem wrong csum : 5891/c98d, len 90 bytes, c0000001fa185102
[ 858.131567] raw data: 00000000: 00 0d 93 43 81 62 d4 3d 7e 4c 48 b7 86 dd 61 01 ...C.b.=~LH...a.
[ 858.131570] raw data: 00000010: 1e b1 00 20 06 40 20 01 0a 62 17 11 88 01 00 00 ... .@ ..b......
[ 858.131572] raw data: 00000020: 00 00 00 00 0a 38 20 01 0a 62 17 11 88 01 00 00 .....8 ..b......
[ 858.131574] raw data: 00000030: 00 00 00 00 00 07 94 b4 00 16 86 f5 2a 04 36 cb ............*.6.
[ 858.131577] raw data: 00000040: 50 a1 80 10 05 93 3e 10 00 00 01 01 08 0a 58 b2 P.....>.......X.
[ 858.131579] raw data: 00000050: de 5b 61 5f 30 52 3f ea 70 9b .[a_0R?.p.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply
* Re: [PATCH 1/3] kbuild: add macro for controlling warnings to linux/compiler.h
From: Paul Burton @ 2018-06-19 19:02 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Arnd Bergmann, Linux Kbuild mailing list, Mauro Carvalho Chehab,
Linux-MIPS, Ingo Molnar, Matthew Wilcox, Thomas Gleixner,
Douglas Anderson, Josh Poimboeuf, Andrew Morton,
Matthias Kaehlcke, He Zhe, Benjamin Herrenschmidt, Michal Marek,
Khem Raj, Christophe Leroy, Al Viro, Stafford Horne,
Gideon Israel Dsouza, Kees Cook, Michael Ellerman, Heiko Carstens,
Linux Kernel Mailing List, Paul Mackerras, linuxppc-dev
In-Reply-To: <CAK7LNASvCha27kU4ipn23uOpNuxkzJrNzWBwYcxN4n=3xtv8SA@mail.gmail.com>
Hi Masahiro,
On Wed, Jun 20, 2018 at 02:34:35AM +0900, Masahiro Yamada wrote:
> 2018-06-16 9:53 GMT+09:00 Paul Burton <paul.burton@mips.com>:
> > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> > index f1a7492a5cc8..aba64a2912d8 100644
> > --- a/include/linux/compiler-gcc.h
> > +++ b/include/linux/compiler-gcc.h
> > @@ -347,3 +347,69 @@
> > #if GCC_VERSION >= 50100
> > #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
> > #endif
> > +
> > +/*
> > + * turn individual warnings and errors on and off locally, depending
> > + * on version.
> > + */
> > +#define __diag_GCC(version, s) __diag_GCC_ ## version(s)
> > +
> > +#if GCC_VERSION >= 40600
> > +#define __diag_str1(s) #s
> > +#define __diag_str(s) __diag_str1(s)
> > +#define __diag(s) _Pragma(__diag_str(GCC diagnostic s))
> > +
> > +/* compilers before gcc-4.6 do not understand "#pragma GCC diagnostic push" */
> > +#define __diag_GCC_4_6(s) __diag(s)
> > +#else
> > +#define __diag(s)
> > +#define __diag_GCC_4_6(s)
> > +#endif
> > +
> > +#if GCC_VERSION >= 40700
> > +#define __diag_GCC_4_7(s) __diag(s)
> > +#else
> > +#define __diag_GCC_4_7(s)
> > +#endif
> > +
> > +#if GCC_VERSION >= 40800
> > +#define __diag_GCC_4_8(s) __diag(s)
> > +#else
> > +#define __diag_GCC_4_8(s)
> > +#endif
> > +
> > +#if GCC_VERSION >= 40900
> > +#define __diag_GCC_4_9(s) __diag(s)
> > +#else
> > +#define __diag_GCC_4_9(s)
> > +#endif
> > +
> > +#if GCC_VERSION >= 50000
> > +#define __diag_GCC_5(s) __diag(s)
> > +#else
> > +#define __diag_GCC_5(s)
> > +#endif
> > +
> > +#if GCC_VERSION >= 60000
> > +#define __diag_GCC_6(s) __diag(s)
> > +#else
> > +#define __diag_GCC_6(s)
> > +#endif
> > +
> > +#if GCC_VERSION >= 70000
> > +#define __diag_GCC_7(s) __diag(s)
> > +#else
> > +#define __diag_GCC_7(s)
> > +#endif
> > +
> > +#if GCC_VERSION >= 80000
> > +#define __diag_GCC_8(s) __diag(s)
> > +#else
> > +#define __diag_GCC_8(s)
> > +#endif
> > +
> > +#if GCC_VERSION >= 90000
> > +#define __diag_GCC_9(s) __diag(s)
> > +#else
> > +#define __diag_GCC_9(s)
> > +#endif
>
>
> Hmm, we would have to add this for every release.
Well, strictly speaking only ones that we need to modify diags for - ie.
in this series we could get away with only adding the GCC 8 macro if we
wanted.
> > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> > index 6b79a9bba9a7..313a2ad884e0 100644
> > --- a/include/linux/compiler_types.h
> > +++ b/include/linux/compiler_types.h
> > @@ -271,4 +271,22 @@ struct ftrace_likely_data {
> > # define __native_word(t) (sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long))
> > #endif
> >
> > +#ifndef __diag
> > +#define __diag(string)
> > +#endif
> > +
> > +#ifndef __diag_GCC
> > +#define __diag_GCC(string)
> > +#endif
>
> __diag_GCC() takes two arguments,
> so it should be:
>
> #ifndef __diag_GCC
> #define __diag_GCC(version, s)
> #endif
>
>
> Otherwise, this would cause warning like this:
>
>
> arch/arm64/kernel/sys.c:40:1: error: macro "__diag_GCC" passed 2
> arguments, but takes just 1
> SYSCALL_DEFINE1(arm64_personality, unsigned int, personality)
> ^~~~~~~~~~
Yes, good catch.
> > +#define __diag_push() __diag(push)
> > +#define __diag_pop() __diag(pop)
> > +
> > +#define __diag_ignore(compiler, version, option, comment) \
> > + __diag_ ## compiler(version, ignored option)
> > +#define __diag_warn(compiler, version, option, comment) \
> > + __diag_ ## compiler(version, warning option)
> > +#define __diag_error(compiler, version, option, comment) \
> > + __diag_ ## compiler(version, error option)
> > +
>
> To me, it looks like this is putting GCC/Clang specific things
> in the common file, <linux/compiler_types.h> .
>
> All compilers must use "ignored", "warning", "error",
> not allowed to use "ignore".
We could move that to linux/compiler-gcc.h pretty easily.
> I also wonder if we could avoid proliferating __diag_GCC_*.
My thought is that it's unlikely we'll ever support enough different
compilers for it to become problematic to list the ones we modify
warnings for in linux/compiler_types.h.
> I attached a bit different implementation below.
>
> I used -Wno-pragmas to avoid unknown option warnings.
That doesn't seem very clean to me because it will hide typos or other
mistakes. One advantage of Arnd's patch is that by specifying the
compiler & version we only attempt to use pragmas that are appropriate
so we don't need to ignore unknown ones.
> Usage is
>
> __diag_push();
> __diag_ignore(-Wattribute-alias,
> "Type aliasing is used to sanitize syscall arguments");
> ...
> __diag_pop();
>
> Comments, ideas are appreciated.
By removing the compiler & version arguments you're enforcing that if we
ignore a warning for one compiler we must ignore it for all, regardless
of whether it's problematic. This essentially presumes that warnings
with the same name will behave the same across compilers, which feels
worse to me than having users of this explicitly state which compilers
need the pragmas.
Thanks,
Paul
^ permalink raw reply
* Re: [PATCH 1/3] kbuild: add macro for controlling warnings to linux/compiler.h
From: Masahiro Yamada @ 2018-06-19 17:34 UTC (permalink / raw)
To: Paul Burton, Arnd Bergmann
Cc: Linux Kbuild mailing list, Mauro Carvalho Chehab, Linux-MIPS,
Ingo Molnar, Matthew Wilcox, Thomas Gleixner, Douglas Anderson,
Josh Poimboeuf, Andrew Morton, Matthias Kaehlcke, He Zhe,
Benjamin Herrenschmidt, Michal Marek, Khem Raj, Christophe Leroy,
Al Viro, Stafford Horne, Gideon Israel Dsouza, Kees Cook,
Michael Ellerman, Heiko Carstens, Linux Kernel Mailing List,
Paul Mackerras, linuxppc-dev
In-Reply-To: <20180616005323.7938-2-paul.burton@mips.com>
Hi.
2018-06-16 9:53 GMT+09:00 Paul Burton <paul.burton@mips.com>:
> From: Arnd Bergmann <arnd@arndb.de>
>
> I have occasionally run into a situation where it would make sense to
> control a compiler warning from a source file rather than doing so from
> a Makefile using the $(cc-disable-warning, ...) or $(cc-option, ...)
> helpers.
>
> The approach here is similar to what glibc uses, using __diag() and
> related macros to encapsulate a _Pragma("GCC diagnostic ...") statement
> that gets turned into the respective "#pragma GCC diagnostic ..." by
> the preprocessor when the macro gets expanded.
>
> Like glibc, I also have an argument to pass the affected compiler
> version, but decided to actually evaluate that one. For now, this
> supports GCC_4_6, GCC_4_7, GCC_4_8, GCC_4_9, GCC_5, GCC_6, GCC_7,
> GCC_8 and GCC_9. Adding support for CLANG_5 and other interesting
> versions is straightforward here. GNU compilers starting with gcc-4.2
> could support it in principle, but "#pragma GCC diagnostic push"
> was only added in gcc-4.6, so it seems simpler to not deal with those
> at all. The same versions show a large number of warnings already,
> so it seems easier to just leave it at that and not do a more
> fine-grained control for them.
>
> The use cases I found so far include:
>
> - turning off the gcc-8 -Wattribute-alias warning inside of the
> SYSCALL_DEFINEx() macro without having to do it globally.
>
> - Reducing the build time for a simple re-make after a change,
> once we move the warnings from ./Makefile and
> ./scripts/Makefile.extrawarn into linux/compiler.h
>
> - More control over the warnings based on other configurations,
> using preprocessor syntax instead of Makefile syntax. This should make
> it easier for the average developer to understand and change things.
>
> - Adding an easy way to turn the W=1 option on unconditionally
> for a subdirectory or a specific file. This has been requested
> by several developers in the past that want to have their subsystems
> W=1 clean.
>
> - Integrating clang better into the build systems. Clang supports
> more warnings than GCC, and we probably want to classify them
> as default, W=1, W=2 etc, but there are cases in which the
> warnings should be classified differently due to excessive false
> positives from one or the other compiler.
>
> - Adding a way to turn the default warnings into errors (e.g. using
> a new "make E=0" tag) while not also turning the W=1 warnings into
> errors.
>
> This patch for now just adds the minimal infrastructure in order to
> do the first of the list above. As the #pragma GCC diagnostic
> takes precedence over command line options, the next step would be
> to convert a lot of the individual Makefiles that set nonstandard
> options to use __diag() instead.
>
> [paul.burton@mips.com:
> - Rebase atop current master.
> - Add __diag_GCC, or more generally __diag_<compiler>, abstraction to
> avoid code outside of linux/compiler-gcc.h needing to duplicate
> knowledge about different GCC versions.
> - Add a comment argument to __diag_{ignore,warn,error} which isn't
> used in the expansion of the macros but serves to push people to
> document the reason for using them - per feedback from Kees Cook.]
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Paul Burton <paul.burton@mips.com>
> Cc: Michal Marek <michal.lkml@markovi.net>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: Matthew Wilcox <matthew@wil.cx>
> Cc: Matthias Kaehlcke <mka@chromium.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Gideon Israel Dsouza <gidisrael@gmail.com>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Stafford Horne <shorne@gmail.com>
> Cc: Khem Raj <raj.khem@gmail.com>
> Cc: He Zhe <zhe.he@windriver.com>
> Cc: linux-kbuild@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-mips@linux-mips.org
> Cc: linuxppc-dev@lists.ozlabs.org
> ---
>
> include/linux/compiler-gcc.h | 66 ++++++++++++++++++++++++++++++++++
> include/linux/compiler_types.h | 18 ++++++++++
> 2 files changed, 84 insertions(+)
>
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index f1a7492a5cc8..aba64a2912d8 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -347,3 +347,69 @@
> #if GCC_VERSION >= 50100
> #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
> #endif
> +
> +/*
> + * turn individual warnings and errors on and off locally, depending
> + * on version.
> + */
> +#define __diag_GCC(version, s) __diag_GCC_ ## version(s)
> +
> +#if GCC_VERSION >= 40600
> +#define __diag_str1(s) #s
> +#define __diag_str(s) __diag_str1(s)
> +#define __diag(s) _Pragma(__diag_str(GCC diagnostic s))
> +
> +/* compilers before gcc-4.6 do not understand "#pragma GCC diagnostic push" */
> +#define __diag_GCC_4_6(s) __diag(s)
> +#else
> +#define __diag(s)
> +#define __diag_GCC_4_6(s)
> +#endif
> +
> +#if GCC_VERSION >= 40700
> +#define __diag_GCC_4_7(s) __diag(s)
> +#else
> +#define __diag_GCC_4_7(s)
> +#endif
> +
> +#if GCC_VERSION >= 40800
> +#define __diag_GCC_4_8(s) __diag(s)
> +#else
> +#define __diag_GCC_4_8(s)
> +#endif
> +
> +#if GCC_VERSION >= 40900
> +#define __diag_GCC_4_9(s) __diag(s)
> +#else
> +#define __diag_GCC_4_9(s)
> +#endif
> +
> +#if GCC_VERSION >= 50000
> +#define __diag_GCC_5(s) __diag(s)
> +#else
> +#define __diag_GCC_5(s)
> +#endif
> +
> +#if GCC_VERSION >= 60000
> +#define __diag_GCC_6(s) __diag(s)
> +#else
> +#define __diag_GCC_6(s)
> +#endif
> +
> +#if GCC_VERSION >= 70000
> +#define __diag_GCC_7(s) __diag(s)
> +#else
> +#define __diag_GCC_7(s)
> +#endif
> +
> +#if GCC_VERSION >= 80000
> +#define __diag_GCC_8(s) __diag(s)
> +#else
> +#define __diag_GCC_8(s)
> +#endif
> +
> +#if GCC_VERSION >= 90000
> +#define __diag_GCC_9(s) __diag(s)
> +#else
> +#define __diag_GCC_9(s)
> +#endif
Hmm, we would have to add this for every release.
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 6b79a9bba9a7..313a2ad884e0 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -271,4 +271,22 @@ struct ftrace_likely_data {
> # define __native_word(t) (sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long))
> #endif
>
> +#ifndef __diag
> +#define __diag(string)
> +#endif
> +
> +#ifndef __diag_GCC
> +#define __diag_GCC(string)
> +#endif
__diag_GCC() takes two arguments,
so it should be:
#ifndef __diag_GCC
#define __diag_GCC(version, s)
#endif
Otherwise, this would cause warning like this:
arch/arm64/kernel/sys.c:40:1: error: macro "__diag_GCC" passed 2
arguments, but takes just 1
SYSCALL_DEFINE1(arm64_personality, unsigned int, personality)
^~~~~~~~~~
> +#define __diag_push() __diag(push)
> +#define __diag_pop() __diag(pop)
> +
> +#define __diag_ignore(compiler, version, option, comment) \
> + __diag_ ## compiler(version, ignored option)
> +#define __diag_warn(compiler, version, option, comment) \
> + __diag_ ## compiler(version, warning option)
> +#define __diag_error(compiler, version, option, comment) \
> + __diag_ ## compiler(version, error option)
> +
To me, it looks like this is putting GCC/Clang specific things
in the common file, <linux/compiler_types.h> .
All compilers must use "ignored", "warning", "error",
not allowed to use "ignore".
I also wonder if we could avoid proliferating __diag_GCC_*.
> #endif /* __LINUX_COMPILER_TYPES_H */
> --
> 2.17.1
>
I attached a bit different implementation below.
I used -Wno-pragmas to avoid unknown option warnings.
diff --git a/Makefile b/Makefile
index ca2af1a..d610d81 100644
--- a/Makefile
+++ b/Makefile
@@ -817,6 +817,8 @@ KBUILD_CFLAGS += $(call cc-option,-Werror=designated-init)
# change __FILE__ to the relative path from the srctree
KBUILD_CFLAGS += $(call cc-option,-fmacro-prefix-map=$(srctree)/=)
+KBUILD_CFLAGS += $(call cc-option,-Wno-pragmas)
+
# use the deterministic mode of AR if available
KBUILD_ARFLAGS := $(call ar-option,D)
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index f1a7492..3f9c1cc 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -3,6 +3,8 @@
#error "Please don't include <linux/compiler-gcc.h> directly, include
<linux/compiler.h> instead."
#endif
+#include <linux/stringify.h>
+
/*
* Common definitions for all gcc versions go here.
*/
@@ -259,6 +261,16 @@
*/
#define __visible __attribute__((externally_visible))
+/* turn individual warnings and errors on and off locally */
+#define __diag_gcc(s) _Pragma(__stringify(GCC diagnostic s))
+
+#define __diag_push() __diag_gcc(push)
+#define __diag_pop() __diag_gcc(pop)
+
+#define __diag_ignore(option, comment) __diag_gcc(ignored __stringify(option))
+#define __diag_warn(option, comment) __diag_gcc(warning __stringify(option))
+#define __diag_error(option, comment) __diag_gcc(error __stringify(option))
+
#endif /* GCC_VERSION >= 40600 */
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 6b79a9b..32e354f 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -271,4 +271,24 @@ struct ftrace_likely_data {
# define __native_word(t) (sizeof(t) == sizeof(char) || sizeof(t) ==
sizeof(short) || sizeof(t) == sizeof(int) || sizeof(t) ==
sizeof(long))
#endif
+#ifndef __diag_push
+#define __diag_push()
+#endif
+
+#ifndef __diag_pop
+#define __diag_pop()
+#endif
+
+#ifndef __diag_ignore
+#define __diag_ignore(option, comment)
+#endif
+
+#ifndef __diag_warn
+#define __diag_warn(option, comment)
+#endif
+
+#ifndef __diag_error
+#define __diag_error(option, comment)
+#endif
+
#endif /* __LINUX_COMPILER_TYPES_H */
Usage is
__diag_push();
__diag_ignore(-Wattribute-alias,
"Type aliasing is used to sanitize syscall arguments");
...
__diag_pop();
Comments, ideas are appreciated.
--
Best Regards
Masahiro Yamada
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox