Re: [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()
On Sat, Aug 29, 2020 at 1:40 PM Guenter Roeck wrote:
>
> Except for
>
> CHECK: spaces preferred around that '+' (ctx:VxV)
> #29: FILE: drivers/dma/fsldma.h:223:
> + u32 val_lo = in_be32((u32 __iomem *)addr+1);
Added spaces.
> I don't see anything wrong with it either, so
>
> Reviewed-by: Guenter Roeck
>
> Since I didn't see the real problem with the original code,
> I'd take that with a grain of salt, though.
Well, honestly, the old code was so confused that just making it build
is clearly already an improvement even if everything else were to be
wrong.
So I committed my "fix". If it turns out there's more wrong in there
and somebody tests it, we can fix it again. But now it hopefully
compiles, at least.
My bet is that if that driver ever worked on ppc32, it will continue
to work whatever we do to that function.
I _think_ the old code happened to - completely by mistake - get the
value right for the case of "little endian access, with dma_addr_t
being 32-bit". Because then it would still read the upper bits wrong,
but the cast to dma_addr_t would then throw those bits away. And the
lower bits would be right.
But for big-endian accesses or for ARCH_DMA_ADDR_T_64BIT it really
looks like it always returned a completely incorrect value.
And again - the driver may have worked even with that completely
incorrect value, since the use of it seems to be very incidental.
In either case ("it didn't work before" or "it worked because the
value doesn't really matter"), I don't think I could possibly have
made things worse.
Famous last words.
Linus
Re: [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()
On 8/29/20 10:29 AM, Linus Torvalds wrote:
> On Sat, Aug 29, 2020 at 5:46 AM Luc Van Oostenryck
> wrote:
>>
>> But the pointer is already 32-bit, so simply cast the pointer to u32.
>
> Yeah, that code was completely pointless. If the pointer had actually
> been 64-bit, the old code would have warned too.
>
> The odd thing is that the fsl_iowrite64() functions make sense. It's
> only the fsl_ioread64() functions that seem to be written by somebody
> who is really confused.
>
> That said, this patch only humors the confusion. The cast to 'u32' is
> completely pointless. In fact, it seems to be actively wrong, because
> it means that the later "fsl_addr + 1" is done entirely incorrectly -
> it now literally adds "1" to an integer value, while the iowrite()
> functions will add one to a "u32 __iomem *" pointer (so will do
> pointer arithmetic, and add 4).
>
Outch.
> So this code has never ever worked correctly to begin with, but the
> patches to fix the warning miss the point. The problem isn't the
> warning, the problem is that the code is broken and completely wrong
> to begin with.
>
> And the "lower_32_bits()" thing has always been pure and utter
> confusion and complete garbage.
>
> I *think* the right patch is the one attached, but since this code is
> clearly utterly broken, I'd want somebody to test it.
>
> It has probably never ever worked on 32-bit powerpc, or did so purely
> by mistake (perhaps because nobody really cares - the only 64-bit use
> is this:
>
> static dma_addr_t get_cdar(struct fsldma_chan *chan)
> {
> return FSL_DMA_IN(chan, &chan->regs->cdar, 64) & ~FSL_DMA_SNEN;
> }
>
> and there are two users of that: one which ignores the return value,
> and one that looks like it might end up half-way working even if the
> value read was garbage (it's used only to compare against a "current
> descriptor" value).
>
> Anyway, the fix is definitely not to just shut up the warning. The
> warning is only a sign of utter confusion in that driver.
>
> Can somebody with the hardware test this on 32-bit ppc?
>
> And if not (judging by just how broken those functions are, maybe it
> never did work), can somebody with a ppc32 setup at least compile-test
> this patch and look at whether it makes sense, in ways the old code
> did not.
>
A bit more careful this time. For the attached patch:
Compile-tested-by: Guenter Roeck
Except for
CHECK: spaces preferred around that '+' (ctx:VxV)
#29: FILE: drivers/dma/fsldma.h:223:
+ u32 val_lo = in_be32((u32 __iomem *)addr+1);
I don't see anything wrong with it either, so
Reviewed-by: Guenter Roeck
Since I didn't see the real problem with the original code,
I'd take that with a grain of salt, though.
Guenter
Re: [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()
On Sat, Aug 29, 2020 at 5:46 AM Luc Van Oostenryck
wrote:
>
> But the pointer is already 32-bit, so simply cast the pointer to u32.
Yeah, that code was completely pointless. If the pointer had actually
been 64-bit, the old code would have warned too.
The odd thing is that the fsl_iowrite64() functions make sense. It's
only the fsl_ioread64() functions that seem to be written by somebody
who is really confused.
That said, this patch only humors the confusion. The cast to 'u32' is
completely pointless. In fact, it seems to be actively wrong, because
it means that the later "fsl_addr + 1" is done entirely incorrectly -
it now literally adds "1" to an integer value, while the iowrite()
functions will add one to a "u32 __iomem *" pointer (so will do
pointer arithmetic, and add 4).
So this code has never ever worked correctly to begin with, but the
patches to fix the warning miss the point. The problem isn't the
warning, the problem is that the code is broken and completely wrong
to begin with.
And the "lower_32_bits()" thing has always been pure and utter
confusion and complete garbage.
I *think* the right patch is the one attached, but since this code is
clearly utterly broken, I'd want somebody to test it.
It has probably never ever worked on 32-bit powerpc, or did so purely
by mistake (perhaps because nobody really cares - the only 64-bit use
is this:
static dma_addr_t get_cdar(struct fsldma_chan *chan)
{
return FSL_DMA_IN(chan, &chan->regs->cdar, 64) & ~FSL_DMA_SNEN;
}
and there are two users of that: one which ignores the return value,
and one that looks like it might end up half-way working even if the
value read was garbage (it's used only to compare against a "current
descriptor" value).
Anyway, the fix is definitely not to just shut up the warning. The
warning is only a sign of utter confusion in that driver.
Can somebody with the hardware test this on 32-bit ppc?
And if not (judging by just how broken those functions are, maybe it
never did work), can somebody with a ppc32 setup at least compile-test
this patch and look at whether it makes sense, in ways the old code
did not.
Linus
patch
Description: Binary data
Re: [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()
Le 29/08/2020 à 17:05, Guenter Roeck a écrit :
On Sat, Aug 29, 2020 at 02:45:38PM +0200, Luc Van Oostenryck wrote:
For ppc32, the functions fsl_ioread64() & fsl_ioread64be()
use lower_32_bits() as a fancy way to cast the pointer to u32
in order to do non-atomic 64-bit IO.
But the pointer is already 32-bit, so simply cast the pointer to u32.
This fixes a compile error introduced by
ef91bb196b0d ("kernel.h: Silence sparse warning in lower_32_bits")
Fixes: ef91bb196b0db1013ef8705367bc2d7944ef696b
checkpatch complains about this and prefers
Fixes: ef91bb196b0d ("kernel.h: Silence sparse warning in lower_32_bits")
Checkpatch also complains about spacing:
CHECK:SPACING: No space is necessary after a cast
#39: FILE: drivers/dma/fsldma.h:208:
+ u32 fsl_addr = (u32) addr;
CHECK:SPACING: No space is necessary after a cast
#48: FILE: drivers/dma/fsldma.h:222:
+ u32 fsl_addr = (u32) addr;
total: 0 errors, 0 warnings, 2 checks, 16 lines checked
Christophe
Otherwise
Tested-by: Guenter Roeck
Reported-by: Guenter Roeck
Cc: Li Yang
Cc: Zhang Wei
Cc: Dan Williams
Cc: Vinod Koul
Cc: Herbert Xu
Cc: linuxppc-dev@lists.ozlabs.org
Cc: dmaeng...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Luc Van Oostenryck
---
drivers/dma/fsldma.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h
index 56f18ae99233..6f6fa7641fa2 100644
--- a/drivers/dma/fsldma.h
+++ b/drivers/dma/fsldma.h
@@ -205,7 +205,7 @@ struct fsldma_chan {
#else
static u64 fsl_ioread64(const u64 __iomem *addr)
{
- u32 fsl_addr = lower_32_bits(addr);
+ u32 fsl_addr = (u32) addr;
u64 fsl_addr_hi = (u64)in_le32((u32 *)(fsl_addr + 1)) << 32;
return fsl_addr_hi | in_le32((u32 *)fsl_addr);
@@ -219,7 +219,7 @@ static void fsl_iowrite64(u64 val, u64 __iomem *addr)
static u64 fsl_ioread64be(const u64 __iomem *addr)
{
- u32 fsl_addr = lower_32_bits(addr);
+ u32 fsl_addr = (u32) addr;
u64 fsl_addr_hi = (u64)in_be32((u32 *)fsl_addr) << 32;
return fsl_addr_hi | in_be32((u32 *)(fsl_addr + 1));
--
2.28.0
Re: [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()
On Sat, Aug 29, 2020 at 02:45:38PM +0200, Luc Van Oostenryck wrote:
> For ppc32, the functions fsl_ioread64() & fsl_ioread64be()
> use lower_32_bits() as a fancy way to cast the pointer to u32
> in order to do non-atomic 64-bit IO.
>
> But the pointer is already 32-bit, so simply cast the pointer to u32.
>
> This fixes a compile error introduced by
>ef91bb196b0d ("kernel.h: Silence sparse warning in lower_32_bits")
>
> Fixes: ef91bb196b0db1013ef8705367bc2d7944ef696b
checkpatch complains about this and prefers
Fixes: ef91bb196b0d ("kernel.h: Silence sparse warning in lower_32_bits")
Otherwise
Tested-by: Guenter Roeck
> Reported-by: Guenter Roeck
> Cc: Li Yang
> Cc: Zhang Wei
> Cc: Dan Williams
> Cc: Vinod Koul
> Cc: Herbert Xu
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: dmaeng...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Signed-off-by: Luc Van Oostenryck
> ---
> drivers/dma/fsldma.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h
> index 56f18ae99233..6f6fa7641fa2 100644
> --- a/drivers/dma/fsldma.h
> +++ b/drivers/dma/fsldma.h
> @@ -205,7 +205,7 @@ struct fsldma_chan {
> #else
> static u64 fsl_ioread64(const u64 __iomem *addr)
> {
> - u32 fsl_addr = lower_32_bits(addr);
> + u32 fsl_addr = (u32) addr;
> u64 fsl_addr_hi = (u64)in_le32((u32 *)(fsl_addr + 1)) << 32;
>
> return fsl_addr_hi | in_le32((u32 *)fsl_addr);
> @@ -219,7 +219,7 @@ static void fsl_iowrite64(u64 val, u64 __iomem *addr)
>
> static u64 fsl_ioread64be(const u64 __iomem *addr)
> {
> - u32 fsl_addr = lower_32_bits(addr);
> + u32 fsl_addr = (u32) addr;
> u64 fsl_addr_hi = (u64)in_be32((u32 *)fsl_addr) << 32;
>
> return fsl_addr_hi | in_be32((u32 *)(fsl_addr + 1));
> --
> 2.28.0
>
[PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()
For ppc32, the functions fsl_ioread64() & fsl_ioread64be()
use lower_32_bits() as a fancy way to cast the pointer to u32
in order to do non-atomic 64-bit IO.
But the pointer is already 32-bit, so simply cast the pointer to u32.
This fixes a compile error introduced by
ef91bb196b0d ("kernel.h: Silence sparse warning in lower_32_bits")
Fixes: ef91bb196b0db1013ef8705367bc2d7944ef696b
Reported-by: Guenter Roeck
Cc: Li Yang
Cc: Zhang Wei
Cc: Dan Williams
Cc: Vinod Koul
Cc: Herbert Xu
Cc: linuxppc-dev@lists.ozlabs.org
Cc: dmaeng...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Luc Van Oostenryck
---
drivers/dma/fsldma.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h
index 56f18ae99233..6f6fa7641fa2 100644
--- a/drivers/dma/fsldma.h
+++ b/drivers/dma/fsldma.h
@@ -205,7 +205,7 @@ struct fsldma_chan {
#else
static u64 fsl_ioread64(const u64 __iomem *addr)
{
- u32 fsl_addr = lower_32_bits(addr);
+ u32 fsl_addr = (u32) addr;
u64 fsl_addr_hi = (u64)in_le32((u32 *)(fsl_addr + 1)) << 32;
return fsl_addr_hi | in_le32((u32 *)fsl_addr);
@@ -219,7 +219,7 @@ static void fsl_iowrite64(u64 val, u64 __iomem *addr)
static u64 fsl_ioread64be(const u64 __iomem *addr)
{
- u32 fsl_addr = lower_32_bits(addr);
+ u32 fsl_addr = (u32) addr;
u64 fsl_addr_hi = (u64)in_be32((u32 *)fsl_addr) << 32;
return fsl_addr_hi | in_be32((u32 *)(fsl_addr + 1));
--
2.28.0
[PATCH net-next] net/wan/fsl_ucc_hdlc: Add MODULE_DESCRIPTION
Add missing MODULE_DESCRIPTION.
Signed-off-by: YueHaibing
---
drivers/net/wan/fsl_ucc_hdlc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
index 9edd94679283..dca97cd7c4e7 100644
--- a/drivers/net/wan/fsl_ucc_hdlc.c
+++ b/drivers/net/wan/fsl_ucc_hdlc.c
@@ -1295,3 +1295,4 @@ static struct platform_driver ucc_hdlc_driver = {
module_platform_driver(ucc_hdlc_driver);
MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION(DRV_DESC);
--
2.17.1
Re: [PATCH 08/10] x86: remove address space overrides using set_fs()
On Thu, Aug 27, 2020 at 11:15:12AM -0700, Linus Torvalds wrote: > > SYM_FUNC_START(__put_user_2) > > - ENTER > > - mov TASK_addr_limit(%_ASM_BX),%_ASM_BX > > + LOAD_TASK_SIZE_MAX > > sub $1,%_ASM_BX > > It's even more obvious here. We load a constant and then immediately > do a "sub $1" on that value. > > It's not a huge deal, you don't have to respin the series for this, I > just wanted to point it out so that people are aware of it and if I > forget somebody else will hopefully remember that "we should fix that > too". The changes seem easy enough and I need to respin at least for the lkdtm changes, and probaby also for a pending fix in the low-level x86 code that will hopefully be picked up for 5.9. But the more important questions is: how do we want to pick the series up? Especially due to the splice changes I really want it to be in linux-next as long as possible.
Re: [PATCH 05/10] lkdtm: disable set_fs-based tests for !CONFIG_SET_FS
On Thu, Aug 27, 2020 at 11:06:28AM -0700, Linus Torvalds wrote: > On Thu, Aug 27, 2020 at 8:00 AM Christoph Hellwig wrote: > > > > Once we can't manipulate the address limit, we also can't test what > > happens when the manipulation is abused. > > Just remove these tests entirely. > > Once set_fs() doesn't exist on x86, the tests no longer make any sense > what-so-ever, because test coverage will be basically zero. > > So don't make the code uglier just to maintain a fiction that > something is tested when it isn't really. Sure fine with me unless Kees screams.
Re: [PATCH 01/10] fs: don't allow kernel reads and writes without iter ops
On Thu, Aug 27, 2020 at 03:58:02PM +, David Laight wrote: > Is there a real justification for that? > For system calls supplying both methods makes sense to avoid > the extra code paths for a simple read/write. Al asked for it as two of our four in-tree instances do have weird semantics, and we can't change that any more. And the other two don't make sense to be used with kernel_read and kernel_write ( (/dev/null and /dev/zero).
