* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:35 ` Christophe Leroy
0 siblings, 0 replies; 77+ messages in thread
From: Christophe Leroy @ 2020-01-08 8:35 UTC (permalink / raw)
To: Krzysztof Kozlowski, Geert Uytterhoeven
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Ben Skeggs, Dave Airlie, Matt Turner, arcml, Nick Kossifidis,
Allen Hubbe, Arnd Bergmann, Jon Mason, Ivan Kokshaysky,
Thomas Gleixner, Mauro Carvalho Chehab, Kalle Valo,
Richard Henderson, Parisc List, netdev, linux-wireless,
Linux Kernel Mailing List, Luis Chamberlain, Vineet Gupta, alpha,
linux-ntb, Andrew Morton, Linux Media Mailing List, linuxppc-dev,
David S. Miller
Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>
>> Hi Krzysztof,
>>
>> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>> The ioread8/16/32() and others have inconsistent interface among the
>>>> architectures: some taking address as const, some not.
>>>>
>>>> It seems there is nothing really stopping all of them to take
>>>> pointer to const.
>>>
>>> Shouldn't all of them take const volatile __iomem pointers?
>>> It seems the "volatile" is missing from all but the implementations in
>>> include/asm-generic/io.h.
>>
>> As my "volatile" comment applies to iowrite*(), too, probably that should be
>> done in a separate patch.
>>
>> Hence with patches 1-5 squashed, and for patches 11-13:
>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> I'll add to this one also changes to ioreadX_rep() and add another
> patch for volatile for reads and writes. I guess your review will be
> appreciated once more because of ioreadX_rep()
>
volatile should really only be used where deemed necessary:
https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
It is said: " ... accessor functions might use volatile on
architectures where direct I/O memory access does work. Essentially,
each accessor call becomes a little critical section on its own and
ensures that the access happens as expected by the programmer."
Christophe
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:35 ` Christophe Leroy
0 siblings, 0 replies; 77+ messages in thread
From: Christophe Leroy @ 2020-01-08 8:35 UTC (permalink / raw)
To: Krzysztof Kozlowski, Geert Uytterhoeven
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Ben Skeggs, Dave Airlie, Matt Turner, arcml, Nick Kossifidis,
Allen Hubbe, Arnd Bergmann, Jon Mason, Ivan Kokshaysky,
Thomas Gleixner, Mauro Carvalho Chehab, Kalle Valo,
Richard Henderson, Parisc List, netdev, linux-wireless,
Linux Kernel Mailing List, Luis Chamberlain, Vineet Gupta,
Daniel Vetter, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>
>> Hi Krzysztof,
>>
>> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>> The ioread8/16/32() and others have inconsistent interface among the
>>>> architectures: some taking address as const, some not.
>>>>
>>>> It seems there is nothing really stopping all of them to take
>>>> pointer to const.
>>>
>>> Shouldn't all of them take const volatile __iomem pointers?
>>> It seems the "volatile" is missing from all but the implementations in
>>> include/asm-generic/io.h.
>>
>> As my "volatile" comment applies to iowrite*(), too, probably that should be
>> done in a separate patch.
>>
>> Hence with patches 1-5 squashed, and for patches 11-13:
>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> I'll add to this one also changes to ioreadX_rep() and add another
> patch for volatile for reads and writes. I guess your review will be
> appreciated once more because of ioreadX_rep()
>
volatile should really only be used where deemed necessary:
https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
It is said: " ... accessor functions might use volatile on
architectures where direct I/O memory access does work. Essentially,
each accessor call becomes a little critical section on its own and
ensures that the access happens as expected by the programmer."
Christophe
_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:35 ` Christophe Leroy
0 siblings, 0 replies; 77+ messages in thread
From: Christophe Leroy @ 2020-01-08 8:35 UTC (permalink / raw)
To: Krzysztof Kozlowski, Geert Uytterhoeven
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Ben Skeggs, Dave Airlie, Matt Turner, arcml, Nick Kossifidis,
Allen Hubbe, Arnd Bergmann, Jon Mason, Ivan Kokshaysky,
Thomas Gleixner, Mauro Carvalho Chehab, Kalle Valo,
Richard Henderson, Parisc List, netdev, linux-wireless,
Linux Kernel Mailing List, Luis Chamberlain, Vineet Gupta,
Daniel Vetter, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>
>> Hi Krzysztof,
>>
>> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>> The ioread8/16/32() and others have inconsistent interface among the
>>>> architectures: some taking address as const, some not.
>>>>
>>>> It seems there is nothing really stopping all of them to take
>>>> pointer to const.
>>>
>>> Shouldn't all of them take const volatile __iomem pointers?
>>> It seems the "volatile" is missing from all but the implementations in
>>> include/asm-generic/io.h.
>>
>> As my "volatile" comment applies to iowrite*(), too, probably that should be
>> done in a separate patch.
>>
>> Hence with patches 1-5 squashed, and for patches 11-13:
>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> I'll add to this one also changes to ioreadX_rep() and add another
> patch for volatile for reads and writes. I guess your review will be
> appreciated once more because of ioreadX_rep()
>
volatile should really only be used where deemed necessary:
https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
It is said: " ... accessor functions might use volatile on
architectures where direct I/O memory access does work. Essentially,
each accessor call becomes a little critical section on its own and
ensures that the access happens as expected by the programmer."
Christophe
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:35 ` Christophe Leroy
0 siblings, 0 replies; 77+ messages in thread
From: Christophe Leroy @ 2020-01-08 8:35 UTC (permalink / raw)
To: Krzysztof Kozlowski, Geert Uytterhoeven
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Ben Skeggs, Dave Airlie, Matt Turner, arcml, Nick Kossifidis,
Allen Hubbe, Arnd Bergmann, Jon
Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>
>> Hi Krzysztof,
>>
>> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>> The ioread8/16/32() and others have inconsistent interface among the
>>>> architectures: some taking address as const, some not.
>>>>
>>>> It seems there is nothing really stopping all of them to take
>>>> pointer to const.
>>>
>>> Shouldn't all of them take const volatile __iomem pointers?
>>> It seems the "volatile" is missing from all but the implementations in
>>> include/asm-generic/io.h.
>>
>> As my "volatile" comment applies to iowrite*(), too, probably that should be
>> done in a separate patch.
>>
>> Hence with patches 1-5 squashed, and for patches 11-13:
>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> I'll add to this one also changes to ioreadX_rep() and add another
> patch for volatile for reads and writes. I guess your review will be
> appreciated once more because of ioreadX_rep()
>
volatile should really only be used where deemed necessary:
https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
It is said: " ... accessor functions might use volatile on
architectures where direct I/O memory access does work. Essentially,
each accessor call becomes a little critical section on its own and
ensures that the access happens as expected by the programmer."
Christophe
_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:35 ` Christophe Leroy
0 siblings, 0 replies; 77+ messages in thread
From: Christophe Leroy @ 2020-01-08 8:35 UTC (permalink / raw)
To: Krzysztof Kozlowski, Geert Uytterhoeven
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, netdev, Paul Mackerras, Linux-Arch,
Dave Jiang, Yoshinori Sato, Helge Deller, Linux-sh list,
Alexey Brodkin, Ben Skeggs, nouveau, Dave Airlie, Matt Turner,
arcml, Nick Kossifidis, Allen Hubbe, Arnd Bergmann, alpha,
Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, Vineet Gupta,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Daniel Vetter, Jon Mason, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>
>> Hi Krzysztof,
>>
>> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>> The ioread8/16/32() and others have inconsistent interface among the
>>>> architectures: some taking address as const, some not.
>>>>
>>>> It seems there is nothing really stopping all of them to take
>>>> pointer to const.
>>>
>>> Shouldn't all of them take const volatile __iomem pointers?
>>> It seems the "volatile" is missing from all but the implementations in
>>> include/asm-generic/io.h.
>>
>> As my "volatile" comment applies to iowrite*(), too, probably that should be
>> done in a separate patch.
>>
>> Hence with patches 1-5 squashed, and for patches 11-13:
>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> I'll add to this one also changes to ioreadX_rep() and add another
> patch for volatile for reads and writes. I guess your review will be
> appreciated once more because of ioreadX_rep()
>
volatile should really only be used where deemed necessary:
https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
It is said: " ... accessor functions might use volatile on
architectures where direct I/O memory access does work. Essentially,
each accessor call becomes a little critical section on its own and
ensures that the access happens as expected by the programmer."
Christophe
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
2020-01-08 8:35 ` Christophe Leroy
` (4 preceding siblings ...)
(?)
@ 2020-01-08 8:43 ` Geert Uytterhoeven
-1 siblings, 0 replies; 77+ messages in thread
From: Geert Uytterhoeven @ 2020-01-08 8:43 UTC (permalink / raw)
To: Christophe Leroy
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
nouveau, DRI Development, virtualization, James E.J. Bottomley,
Paul Mackerras, Linux-Arch, Dave Jiang, Yoshinori Sato,
Helge Deller, Linux-sh list, Alexey Brodkin, Krzysztof Kozlowski,
Ben Skeggs, Dave Airlie, Matt Turner, arcml, Nick Kossifidis,
Allen Hubbe, Arnd Bergmann, Jo
Hi Christophe,
On Wed, Jan 8, 2020 at 9:35 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>>> The ioread8/16/32() and others have inconsistent interface among the
> >>>> architectures: some taking address as const, some not.
> >>>>
> >>>> It seems there is nothing really stopping all of them to take
> >>>> pointer to const.
> >>>
> >>> Shouldn't all of them take const volatile __iomem pointers?
> >>> It seems the "volatile" is missing from all but the implementations in
> >>> include/asm-generic/io.h.
> >>
> >> As my "volatile" comment applies to iowrite*(), too, probably that should be
> >> done in a separate patch.
> >>
> >> Hence with patches 1-5 squashed, and for patches 11-13:
> >> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > I'll add to this one also changes to ioreadX_rep() and add another
> > patch for volatile for reads and writes. I guess your review will be
> > appreciated once more because of ioreadX_rep()
>
> volatile should really only be used where deemed necessary:
>
> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>
> It is said: " ... accessor functions might use volatile on
> architectures where direct I/O memory access does work. Essentially,
> each accessor call becomes a little critical section on its own and
> ensures that the access happens as expected by the programmer."
That is exactly the use case here: all above are accessor functions.
Why would ioreadX() not need volatile, while readY() does?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
2020-01-08 8:35 ` Christophe Leroy
` (4 preceding siblings ...)
(?)
@ 2020-01-08 8:43 ` Geert Uytterhoeven
-1 siblings, 0 replies; 77+ messages in thread
From: Geert Uytterhoeven @ 2020-01-08 8:43 UTC (permalink / raw)
To: Christophe Leroy
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Krzysztof Kozlowski, Ben Skeggs, Dave Airlie, Matt Turner, arcml,
Nick Kossifidis, Allen Hubbe, Arnd Bergmann, Jon Mason,
Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, netdev,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Vineet Gupta, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
Hi Christophe,
On Wed, Jan 8, 2020 at 9:35 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>>> The ioread8/16/32() and others have inconsistent interface among the
> >>>> architectures: some taking address as const, some not.
> >>>>
> >>>> It seems there is nothing really stopping all of them to take
> >>>> pointer to const.
> >>>
> >>> Shouldn't all of them take const volatile __iomem pointers?
> >>> It seems the "volatile" is missing from all but the implementations in
> >>> include/asm-generic/io.h.
> >>
> >> As my "volatile" comment applies to iowrite*(), too, probably that should be
> >> done in a separate patch.
> >>
> >> Hence with patches 1-5 squashed, and for patches 11-13:
> >> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > I'll add to this one also changes to ioreadX_rep() and add another
> > patch for volatile for reads and writes. I guess your review will be
> > appreciated once more because of ioreadX_rep()
>
> volatile should really only be used where deemed necessary:
>
> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>
> It is said: " ... accessor functions might use volatile on
> architectures where direct I/O memory access does work. Essentially,
> each accessor call becomes a little critical section on its own and
> ensures that the access happens as expected by the programmer."
That is exactly the use case here: all above are accessor functions.
Why would ioreadX() not need volatile, while readY() does?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:43 ` Geert Uytterhoeven
0 siblings, 0 replies; 77+ messages in thread
From: Geert Uytterhoeven @ 2020-01-08 8:43 UTC (permalink / raw)
To: Christophe Leroy
Cc: Krzysztof Kozlowski, Rich Felker, Jiri Slaby, Michael S. Tsirkin,
David Airlie, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, netdev, Paul Mackerras, Linux-Arch,
Dave Jiang, Yoshinori Sato, Helge Deller, Linux-sh list,
Alexey Brodkin, Ben Skeggs, nouveau, Dave Airlie
Hi Christophe,
On Wed, Jan 8, 2020 at 9:35 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>>> The ioread8/16/32() and others have inconsistent interface among the
> >>>> architectures: some taking address as const, some not.
> >>>>
> >>>> It seems there is nothing really stopping all of them to take
> >>>> pointer to const.
> >>>
> >>> Shouldn't all of them take const volatile __iomem pointers?
> >>> It seems the "volatile" is missing from all but the implementations in
> >>> include/asm-generic/io.h.
> >>
> >> As my "volatile" comment applies to iowrite*(), too, probably that should be
> >> done in a separate patch.
> >>
> >> Hence with patches 1-5 squashed, and for patches 11-13:
> >> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > I'll add to this one also changes to ioreadX_rep() and add another
> > patch for volatile for reads and writes. I guess your review will be
> > appreciated once more because of ioreadX_rep()
>
> volatile should really only be used where deemed necessary:
>
> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>
> It is said: " ... accessor functions might use volatile on
> architectures where direct I/O memory access does work. Essentially,
> each accessor call becomes a little critical section on its own and
> ensures that the access happens as expected by the programmer."
That is exactly the use case here: all above are accessor functions.
Why would ioreadX() not need volatile, while readY() does?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:43 ` Geert Uytterhoeven
0 siblings, 0 replies; 77+ messages in thread
From: Geert Uytterhoeven @ 2020-01-08 8:43 UTC (permalink / raw)
To: Christophe Leroy
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Krzysztof Kozlowski, Ben Skeggs, Dave Airlie, Matt Turner, arcml,
Nick Kossifidis, Allen Hubbe, Arnd Bergmann, Jon Mason,
Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, netdev,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Vineet Gupta, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
Hi Christophe,
On Wed, Jan 8, 2020 at 9:35 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>>> The ioread8/16/32() and others have inconsistent interface among the
> >>>> architectures: some taking address as const, some not.
> >>>>
> >>>> It seems there is nothing really stopping all of them to take
> >>>> pointer to const.
> >>>
> >>> Shouldn't all of them take const volatile __iomem pointers?
> >>> It seems the "volatile" is missing from all but the implementations in
> >>> include/asm-generic/io.h.
> >>
> >> As my "volatile" comment applies to iowrite*(), too, probably that should be
> >> done in a separate patch.
> >>
> >> Hence with patches 1-5 squashed, and for patches 11-13:
> >> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > I'll add to this one also changes to ioreadX_rep() and add another
> > patch for volatile for reads and writes. I guess your review will be
> > appreciated once more because of ioreadX_rep()
>
> volatile should really only be used where deemed necessary:
>
> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>
> It is said: " ... accessor functions might use volatile on
> architectures where direct I/O memory access does work. Essentially,
> each accessor call becomes a little critical section on its own and
> ensures that the access happens as expected by the programmer."
That is exactly the use case here: all above are accessor functions.
Why would ioreadX() not need volatile, while readY() does?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:43 ` Geert Uytterhoeven
0 siblings, 0 replies; 77+ messages in thread
From: Geert Uytterhoeven @ 2020-01-08 8:43 UTC (permalink / raw)
To: Christophe Leroy
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Krzysztof Kozlowski, Ben Skeggs, Dave Airlie, Matt Turner, arcml,
Nick Kossifidis, Allen Hubbe, Arnd Bergmann, Jon Mason,
Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, netdev,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Vineet Gupta, Daniel Vetter, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
Hi Christophe,
On Wed, Jan 8, 2020 at 9:35 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>>> The ioread8/16/32() and others have inconsistent interface among the
> >>>> architectures: some taking address as const, some not.
> >>>>
> >>>> It seems there is nothing really stopping all of them to take
> >>>> pointer to const.
> >>>
> >>> Shouldn't all of them take const volatile __iomem pointers?
> >>> It seems the "volatile" is missing from all but the implementations in
> >>> include/asm-generic/io.h.
> >>
> >> As my "volatile" comment applies to iowrite*(), too, probably that should be
> >> done in a separate patch.
> >>
> >> Hence with patches 1-5 squashed, and for patches 11-13:
> >> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > I'll add to this one also changes to ioreadX_rep() and add another
> > patch for volatile for reads and writes. I guess your review will be
> > appreciated once more because of ioreadX_rep()
>
> volatile should really only be used where deemed necessary:
>
> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>
> It is said: " ... accessor functions might use volatile on
> architectures where direct I/O memory access does work. Essentially,
> each accessor call becomes a little critical section on its own and
> ensures that the access happens as expected by the programmer."
That is exactly the use case here: all above are accessor functions.
Why would ioreadX() not need volatile, while readY() does?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:43 ` Geert Uytterhoeven
0 siblings, 0 replies; 77+ messages in thread
From: Geert Uytterhoeven @ 2020-01-08 8:43 UTC (permalink / raw)
To: Christophe Leroy
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Krzysztof Kozlowski, Ben Skeggs, Dave Airlie, Matt Turner, arcml,
Nick Kossifidis, Allen Hubbe, Arnd Bergmann, Jon Mason,
Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, netdev,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Vineet Gupta, Daniel Vetter, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
Hi Christophe,
On Wed, Jan 8, 2020 at 9:35 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>>> The ioread8/16/32() and others have inconsistent interface among the
> >>>> architectures: some taking address as const, some not.
> >>>>
> >>>> It seems there is nothing really stopping all of them to take
> >>>> pointer to const.
> >>>
> >>> Shouldn't all of them take const volatile __iomem pointers?
> >>> It seems the "volatile" is missing from all but the implementations in
> >>> include/asm-generic/io.h.
> >>
> >> As my "volatile" comment applies to iowrite*(), too, probably that should be
> >> done in a separate patch.
> >>
> >> Hence with patches 1-5 squashed, and for patches 11-13:
> >> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > I'll add to this one also changes to ioreadX_rep() and add another
> > patch for volatile for reads and writes. I guess your review will be
> > appreciated once more because of ioreadX_rep()
>
> volatile should really only be used where deemed necessary:
>
> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>
> It is said: " ... accessor functions might use volatile on
> architectures where direct I/O memory access does work. Essentially,
> each accessor call becomes a little critical section on its own and
> ensures that the access happens as expected by the programmer."
That is exactly the use case here: all above are accessor functions.
Why would ioreadX() not need volatile, while readY() does?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:43 ` Geert Uytterhoeven
0 siblings, 0 replies; 77+ messages in thread
From: Geert Uytterhoeven @ 2020-01-08 8:43 UTC (permalink / raw)
To: Christophe Leroy
Cc: Krzysztof Kozlowski, Rich Felker, Jiri Slaby, Michael S. Tsirkin,
David Airlie, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, netdev, Paul Mackerras, Linux-Arch,
Dave Jiang, Yoshinori Sato, Helge Deller, Linux-sh list,
Alexey Brodkin, Ben Skeggs, nouveau
Hi Christophe,
On Wed, Jan 8, 2020 at 9:35 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>>> The ioread8/16/32() and others have inconsistent interface among the
> >>>> architectures: some taking address as const, some not.
> >>>>
> >>>> It seems there is nothing really stopping all of them to take
> >>>> pointer to const.
> >>>
> >>> Shouldn't all of them take const volatile __iomem pointers?
> >>> It seems the "volatile" is missing from all but the implementations in
> >>> include/asm-generic/io.h.
> >>
> >> As my "volatile" comment applies to iowrite*(), too, probably that should be
> >> done in a separate patch.
> >>
> >> Hence with patches 1-5 squashed, and for patches 11-13:
> >> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > I'll add to this one also changes to ioreadX_rep() and add another
> > patch for volatile for reads and writes. I guess your review will be
> > appreciated once more because of ioreadX_rep()
>
> volatile should really only be used where deemed necessary:
>
> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>
> It is said: " ... accessor functions might use volatile on
> architectures where direct I/O memory access does work. Essentially,
> each accessor call becomes a little critical section on its own and
> ensures that the access happens as expected by the programmer."
That is exactly the use case here: all above are accessor functions.
Why would ioreadX() not need volatile, while readY() does?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:43 ` Geert Uytterhoeven
0 siblings, 0 replies; 77+ messages in thread
From: Geert Uytterhoeven @ 2020-01-08 8:43 UTC (permalink / raw)
To: Christophe Leroy
Cc: Krzysztof Kozlowski, Rich Felker, Jiri Slaby, Michael S. Tsirkin,
David Airlie, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, netdev, Paul Mackerras, Linux-Arch,
Dave Jiang, Yoshinori Sato, Helge Deller, Linux-sh list,
Alexey Brodkin, Ben Skeggs, nouveau, Dave Airlie, Matt Turner,
arcml, Nick Kossifidis, Allen Hubbe, Arnd Bergmann, alpha,
Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, Vineet Gupta,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Daniel Vetter, Jon Mason, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
Hi Christophe,
On Wed, Jan 8, 2020 at 9:35 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>>> The ioread8/16/32() and others have inconsistent interface among the
> >>>> architectures: some taking address as const, some not.
> >>>>
> >>>> It seems there is nothing really stopping all of them to take
> >>>> pointer to const.
> >>>
> >>> Shouldn't all of them take const volatile __iomem pointers?
> >>> It seems the "volatile" is missing from all but the implementations in
> >>> include/asm-generic/io.h.
> >>
> >> As my "volatile" comment applies to iowrite*(), too, probably that should be
> >> done in a separate patch.
> >>
> >> Hence with patches 1-5 squashed, and for patches 11-13:
> >> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > I'll add to this one also changes to ioreadX_rep() and add another
> > patch for volatile for reads and writes. I guess your review will be
> > appreciated once more because of ioreadX_rep()
>
> volatile should really only be used where deemed necessary:
>
> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>
> It is said: " ... accessor functions might use volatile on
> architectures where direct I/O memory access does work. Essentially,
> each accessor call becomes a little critical section on its own and
> ensures that the access happens as expected by the programmer."
That is exactly the use case here: all above are accessor functions.
Why would ioreadX() not need volatile, while readY() does?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
2020-01-08 8:43 ` Geert Uytterhoeven
` (5 preceding siblings ...)
(?)
@ 2020-01-08 8:48 ` Christophe Leroy
-1 siblings, 0 replies; 77+ messages in thread
From: Christophe Leroy @ 2020-01-08 8:48 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
nouveau, DRI Development, virtualization, James E.J. Bottomley,
Paul Mackerras, Linux-Arch, Dave Jiang, Yoshinori Sato,
Helge Deller, Linux-sh list, Alexey Brodkin, Krzysztof Kozlowski,
Ben Skeggs, Dave Airlie, Matt Turner, arcml, Nick Kossifidis,
Allen Hubbe, Arnd Bergmann, Jo
Hi Geert,
Le 08/01/2020 à 09:43, Geert Uytterhoeven a écrit :
> Hi Christophe,
>
> On Wed, Jan 8, 2020 at 9:35 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
>>> On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>>> The ioread8/16/32() and others have inconsistent interface among the
>>>>>> architectures: some taking address as const, some not.
>>>>>>
>>>>>> It seems there is nothing really stopping all of them to take
>>>>>> pointer to const.
>>>>>
>>>>> Shouldn't all of them take const volatile __iomem pointers?
>>>>> It seems the "volatile" is missing from all but the implementations in
>>>>> include/asm-generic/io.h.
>>>>
>>>> As my "volatile" comment applies to iowrite*(), too, probably that should be
>>>> done in a separate patch.
>>>>
>>>> Hence with patches 1-5 squashed, and for patches 11-13:
>>>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>
>>> I'll add to this one also changes to ioreadX_rep() and add another
>>> patch for volatile for reads and writes. I guess your review will be
>>> appreciated once more because of ioreadX_rep()
>>
>> volatile should really only be used where deemed necessary:
>>
>> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>>
>> It is said: " ... accessor functions might use volatile on
>> architectures where direct I/O memory access does work. Essentially,
>> each accessor call becomes a little critical section on its own and
>> ensures that the access happens as expected by the programmer."
>
> That is exactly the use case here: all above are accessor functions.
>
> Why would ioreadX() not need volatile, while readY() does?
>
My point was: it might be necessary for some arches and not for others.
And as pointed by Arnd, the volatile is really only necessary for the
dereference itself, should the arch use dereferencing.
So I guess the best would be to go in the other direction: remove
volatile keyword wherever possible instead of adding it where it is not
needed.
Christophe
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
2020-01-08 8:43 ` Geert Uytterhoeven
` (4 preceding siblings ...)
(?)
@ 2020-01-08 8:48 ` Christophe Leroy
-1 siblings, 0 replies; 77+ messages in thread
From: Christophe Leroy @ 2020-01-08 8:48 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Krzysztof Kozlowski, Ben Skeggs, Dave Airlie, Matt Turner, arcml,
Nick Kossifidis, Allen Hubbe, Arnd Bergmann, Jon Mason,
Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, netdev,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Vineet Gupta, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
Hi Geert,
Le 08/01/2020 à 09:43, Geert Uytterhoeven a écrit :
> Hi Christophe,
>
> On Wed, Jan 8, 2020 at 9:35 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
>>> On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>>> The ioread8/16/32() and others have inconsistent interface among the
>>>>>> architectures: some taking address as const, some not.
>>>>>>
>>>>>> It seems there is nothing really stopping all of them to take
>>>>>> pointer to const.
>>>>>
>>>>> Shouldn't all of them take const volatile __iomem pointers?
>>>>> It seems the "volatile" is missing from all but the implementations in
>>>>> include/asm-generic/io.h.
>>>>
>>>> As my "volatile" comment applies to iowrite*(), too, probably that should be
>>>> done in a separate patch.
>>>>
>>>> Hence with patches 1-5 squashed, and for patches 11-13:
>>>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>
>>> I'll add to this one also changes to ioreadX_rep() and add another
>>> patch for volatile for reads and writes. I guess your review will be
>>> appreciated once more because of ioreadX_rep()
>>
>> volatile should really only be used where deemed necessary:
>>
>> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>>
>> It is said: " ... accessor functions might use volatile on
>> architectures where direct I/O memory access does work. Essentially,
>> each accessor call becomes a little critical section on its own and
>> ensures that the access happens as expected by the programmer."
>
> That is exactly the use case here: all above are accessor functions.
>
> Why would ioreadX() not need volatile, while readY() does?
>
My point was: it might be necessary for some arches and not for others.
And as pointed by Arnd, the volatile is really only necessary for the
dereference itself, should the arch use dereferencing.
So I guess the best would be to go in the other direction: remove
volatile keyword wherever possible instead of adding it where it is not
needed.
Christophe
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:48 ` Christophe Leroy
0 siblings, 0 replies; 77+ messages in thread
From: Christophe Leroy @ 2020-01-08 8:48 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Krzysztof Kozlowski, Rich Felker, Jiri Slaby, Michael S. Tsirkin,
David Airlie, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, netdev, Paul Mackerras, Linux-Arch,
Dave Jiang, Yoshinori Sato, Helge Deller, Linux-sh list,
Alexey Brodkin, Ben Skeggs, nouveau, Dave Airlie
Hi Geert,
Le 08/01/2020 à 09:43, Geert Uytterhoeven a écrit :
> Hi Christophe,
>
> On Wed, Jan 8, 2020 at 9:35 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
>>> On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>>> The ioread8/16/32() and others have inconsistent interface among the
>>>>>> architectures: some taking address as const, some not.
>>>>>>
>>>>>> It seems there is nothing really stopping all of them to take
>>>>>> pointer to const.
>>>>>
>>>>> Shouldn't all of them take const volatile __iomem pointers?
>>>>> It seems the "volatile" is missing from all but the implementations in
>>>>> include/asm-generic/io.h.
>>>>
>>>> As my "volatile" comment applies to iowrite*(), too, probably that should be
>>>> done in a separate patch.
>>>>
>>>> Hence with patches 1-5 squashed, and for patches 11-13:
>>>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>
>>> I'll add to this one also changes to ioreadX_rep() and add another
>>> patch for volatile for reads and writes. I guess your review will be
>>> appreciated once more because of ioreadX_rep()
>>
>> volatile should really only be used where deemed necessary:
>>
>> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>>
>> It is said: " ... accessor functions might use volatile on
>> architectures where direct I/O memory access does work. Essentially,
>> each accessor call becomes a little critical section on its own and
>> ensures that the access happens as expected by the programmer."
>
> That is exactly the use case here: all above are accessor functions.
>
> Why would ioreadX() not need volatile, while readY() does?
>
My point was: it might be necessary for some arches and not for others.
And as pointed by Arnd, the volatile is really only necessary for the
dereference itself, should the arch use dereferencing.
So I guess the best would be to go in the other direction: remove
volatile keyword wherever possible instead of adding it where it is not
needed.
Christophe
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:48 ` Christophe Leroy
0 siblings, 0 replies; 77+ messages in thread
From: Christophe Leroy @ 2020-01-08 8:48 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Krzysztof Kozlowski, Ben Skeggs, Dave Airlie, Matt Turner, arcml,
Nick Kossifidis, Allen Hubbe, Arnd Bergmann, Jon Mason,
Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, netdev,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Vineet Gupta, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
Hi Geert,
Le 08/01/2020 à 09:43, Geert Uytterhoeven a écrit :
> Hi Christophe,
>
> On Wed, Jan 8, 2020 at 9:35 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
>>> On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>>> The ioread8/16/32() and others have inconsistent interface among the
>>>>>> architectures: some taking address as const, some not.
>>>>>>
>>>>>> It seems there is nothing really stopping all of them to take
>>>>>> pointer to const.
>>>>>
>>>>> Shouldn't all of them take const volatile __iomem pointers?
>>>>> It seems the "volatile" is missing from all but the implementations in
>>>>> include/asm-generic/io.h.
>>>>
>>>> As my "volatile" comment applies to iowrite*(), too, probably that should be
>>>> done in a separate patch.
>>>>
>>>> Hence with patches 1-5 squashed, and for patches 11-13:
>>>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>
>>> I'll add to this one also changes to ioreadX_rep() and add another
>>> patch for volatile for reads and writes. I guess your review will be
>>> appreciated once more because of ioreadX_rep()
>>
>> volatile should really only be used where deemed necessary:
>>
>> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>>
>> It is said: " ... accessor functions might use volatile on
>> architectures where direct I/O memory access does work. Essentially,
>> each accessor call becomes a little critical section on its own and
>> ensures that the access happens as expected by the programmer."
>
> That is exactly the use case here: all above are accessor functions.
>
> Why would ioreadX() not need volatile, while readY() does?
>
My point was: it might be necessary for some arches and not for others.
And as pointed by Arnd, the volatile is really only necessary for the
dereference itself, should the arch use dereferencing.
So I guess the best would be to go in the other direction: remove
volatile keyword wherever possible instead of adding it where it is not
needed.
Christophe
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:48 ` Christophe Leroy
0 siblings, 0 replies; 77+ messages in thread
From: Christophe Leroy @ 2020-01-08 8:48 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Krzysztof Kozlowski, Ben Skeggs, Dave Airlie, Matt Turner, arcml,
Nick Kossifidis, Allen Hubbe, Arnd Bergmann, Jon Mason,
Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, netdev,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Vineet Gupta, Daniel Vetter, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
Hi Geert,
Le 08/01/2020 à 09:43, Geert Uytterhoeven a écrit :
> Hi Christophe,
>
> On Wed, Jan 8, 2020 at 9:35 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
>>> On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>>> The ioread8/16/32() and others have inconsistent interface among the
>>>>>> architectures: some taking address as const, some not.
>>>>>>
>>>>>> It seems there is nothing really stopping all of them to take
>>>>>> pointer to const.
>>>>>
>>>>> Shouldn't all of them take const volatile __iomem pointers?
>>>>> It seems the "volatile" is missing from all but the implementations in
>>>>> include/asm-generic/io.h.
>>>>
>>>> As my "volatile" comment applies to iowrite*(), too, probably that should be
>>>> done in a separate patch.
>>>>
>>>> Hence with patches 1-5 squashed, and for patches 11-13:
>>>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>
>>> I'll add to this one also changes to ioreadX_rep() and add another
>>> patch for volatile for reads and writes. I guess your review will be
>>> appreciated once more because of ioreadX_rep()
>>
>> volatile should really only be used where deemed necessary:
>>
>> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>>
>> It is said: " ... accessor functions might use volatile on
>> architectures where direct I/O memory access does work. Essentially,
>> each accessor call becomes a little critical section on its own and
>> ensures that the access happens as expected by the programmer."
>
> That is exactly the use case here: all above are accessor functions.
>
> Why would ioreadX() not need volatile, while readY() does?
>
My point was: it might be necessary for some arches and not for others.
And as pointed by Arnd, the volatile is really only necessary for the
dereference itself, should the arch use dereferencing.
So I guess the best would be to go in the other direction: remove
volatile keyword wherever possible instead of adding it where it is not
needed.
Christophe
_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:48 ` Christophe Leroy
0 siblings, 0 replies; 77+ messages in thread
From: Christophe Leroy @ 2020-01-08 8:48 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Krzysztof Kozlowski, Ben Skeggs, Dave Airlie, Matt Turner, arcml,
Nick Kossifidis, Allen Hubbe, Arnd Bergmann, Jon Mason,
Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, netdev,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Vineet Gupta, Daniel Vetter, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
Hi Geert,
Le 08/01/2020 à 09:43, Geert Uytterhoeven a écrit :
> Hi Christophe,
>
> On Wed, Jan 8, 2020 at 9:35 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
>>> On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>>> The ioread8/16/32() and others have inconsistent interface among the
>>>>>> architectures: some taking address as const, some not.
>>>>>>
>>>>>> It seems there is nothing really stopping all of them to take
>>>>>> pointer to const.
>>>>>
>>>>> Shouldn't all of them take const volatile __iomem pointers?
>>>>> It seems the "volatile" is missing from all but the implementations in
>>>>> include/asm-generic/io.h.
>>>>
>>>> As my "volatile" comment applies to iowrite*(), too, probably that should be
>>>> done in a separate patch.
>>>>
>>>> Hence with patches 1-5 squashed, and for patches 11-13:
>>>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>
>>> I'll add to this one also changes to ioreadX_rep() and add another
>>> patch for volatile for reads and writes. I guess your review will be
>>> appreciated once more because of ioreadX_rep()
>>
>> volatile should really only be used where deemed necessary:
>>
>> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>>
>> It is said: " ... accessor functions might use volatile on
>> architectures where direct I/O memory access does work. Essentially,
>> each accessor call becomes a little critical section on its own and
>> ensures that the access happens as expected by the programmer."
>
> That is exactly the use case here: all above are accessor functions.
>
> Why would ioreadX() not need volatile, while readY() does?
>
My point was: it might be necessary for some arches and not for others.
And as pointed by Arnd, the volatile is really only necessary for the
dereference itself, should the arch use dereferencing.
So I guess the best would be to go in the other direction: remove
volatile keyword wherever possible instead of adding it where it is not
needed.
Christophe
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:48 ` Christophe Leroy
0 siblings, 0 replies; 77+ messages in thread
From: Christophe Leroy @ 2020-01-08 8:48 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Krzysztof Kozlowski, Rich Felker, Jiri Slaby, Michael S. Tsirkin,
David Airlie, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, netdev, Paul Mackerras, Linux-Arch,
Dave Jiang, Yoshinori Sato, Helge Deller, Linux-sh list,
Alexey Brodkin, Ben Skeggs, nouveau
Hi Geert,
Le 08/01/2020 à 09:43, Geert Uytterhoeven a écrit :
> Hi Christophe,
>
> On Wed, Jan 8, 2020 at 9:35 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
>>> On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>>> The ioread8/16/32() and others have inconsistent interface among the
>>>>>> architectures: some taking address as const, some not.
>>>>>>
>>>>>> It seems there is nothing really stopping all of them to take
>>>>>> pointer to const.
>>>>>
>>>>> Shouldn't all of them take const volatile __iomem pointers?
>>>>> It seems the "volatile" is missing from all but the implementations in
>>>>> include/asm-generic/io.h.
>>>>
>>>> As my "volatile" comment applies to iowrite*(), too, probably that should be
>>>> done in a separate patch.
>>>>
>>>> Hence with patches 1-5 squashed, and for patches 11-13:
>>>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>
>>> I'll add to this one also changes to ioreadX_rep() and add another
>>> patch for volatile for reads and writes. I guess your review will be
>>> appreciated once more because of ioreadX_rep()
>>
>> volatile should really only be used where deemed necessary:
>>
>> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>>
>> It is said: " ... accessor functions might use volatile on
>> architectures where direct I/O memory access does work. Essentially,
>> each accessor call becomes a little critical section on its own and
>> ensures that the access happens as expected by the programmer."
>
> That is exactly the use case here: all above are accessor functions.
>
> Why would ioreadX() not need volatile, while readY() does?
>
My point was: it might be necessary for some arches and not for others.
And as pointed by Arnd, the volatile is really only necessary for the
dereference itself, should the arch use dereferencing.
So I guess the best would be to go in the other direction: remove
volatile keyword wherever possible instead of adding it where it is not
needed.
Christophe
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:48 ` Christophe Leroy
0 siblings, 0 replies; 77+ messages in thread
From: Christophe Leroy @ 2020-01-08 8:48 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Krzysztof Kozlowski, Rich Felker, Jiri Slaby, Michael S. Tsirkin,
David Airlie, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, netdev, Paul Mackerras, Linux-Arch,
Dave Jiang, Yoshinori Sato, Helge Deller, Linux-sh list,
Alexey Brodkin, Ben Skeggs, nouveau, Dave Airlie, Matt Turner,
arcml, Nick Kossifidis, Allen Hubbe, Arnd Bergmann, alpha,
Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, Vineet Gupta,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Daniel Vetter, Jon Mason, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
Hi Geert,
Le 08/01/2020 à 09:43, Geert Uytterhoeven a écrit :
> Hi Christophe,
>
> On Wed, Jan 8, 2020 at 9:35 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
>>> On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>>> The ioread8/16/32() and others have inconsistent interface among the
>>>>>> architectures: some taking address as const, some not.
>>>>>>
>>>>>> It seems there is nothing really stopping all of them to take
>>>>>> pointer to const.
>>>>>
>>>>> Shouldn't all of them take const volatile __iomem pointers?
>>>>> It seems the "volatile" is missing from all but the implementations in
>>>>> include/asm-generic/io.h.
>>>>
>>>> As my "volatile" comment applies to iowrite*(), too, probably that should be
>>>> done in a separate patch.
>>>>
>>>> Hence with patches 1-5 squashed, and for patches 11-13:
>>>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>
>>> I'll add to this one also changes to ioreadX_rep() and add another
>>> patch for volatile for reads and writes. I guess your review will be
>>> appreciated once more because of ioreadX_rep()
>>
>> volatile should really only be used where deemed necessary:
>>
>> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>>
>> It is said: " ... accessor functions might use volatile on
>> architectures where direct I/O memory access does work. Essentially,
>> each accessor call becomes a little critical section on its own and
>> ensures that the access happens as expected by the programmer."
>
> That is exactly the use case here: all above are accessor functions.
>
> Why would ioreadX() not need volatile, while readY() does?
>
My point was: it might be necessary for some arches and not for others.
And as pointed by Arnd, the volatile is really only necessary for the
dereference itself, should the arch use dereferencing.
So I guess the best would be to go in the other direction: remove
volatile keyword wherever possible instead of adding it where it is not
needed.
Christophe
^ permalink raw reply [flat|nested] 77+ messages in thread
* RE: [RFT 00/13] iomap: Constify ioreadX() iomem argument
2020-01-08 8:48 ` Christophe Leroy
` (2 preceding siblings ...)
(?)
@ 2020-01-08 17:39 ` David Laight
-1 siblings, 0 replies; 77+ messages in thread
From: David Laight @ 2020-01-08 17:39 UTC (permalink / raw)
To: 'Christophe Leroy', Geert Uytterhoeven
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
nouveau, DRI Development, virtualization, James E.J. Bottomley,
Paul Mackerras, Linux-Arch, Dave Jiang, Yoshinori Sato,
Helge Deller, Linux-sh list, Alexey Brodkin, Krzysztof Kozlowski,
Ben Skeggs, Dave Airlie, Matt Turner, arcml, Nick Kossifidis
From: Christophe Leroy
> Sent: 08 January 2020 08:49
...
> And as pointed by Arnd, the volatile is really only necessary for the
> dereference itself, should the arch use dereferencing.
I've had trouble with some versions of gcc and reading of 'volatile unsigned char *'.
It tended to follow the memory read with an extra mask with 0xff.
(I suspect that internally the value landed into a temporary 'int' variable.)
I got better code using memory barriers.
So putting an asm barrier for the exact location of the memory read
either side of the read should have the desired effect without adding
extra instructions.
(You might think 'volatile' would mean that - but it doesn't.)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 77+ messages in thread
* RE: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 17:39 ` David Laight
0 siblings, 0 replies; 77+ messages in thread
From: David Laight @ 2020-01-08 17:39 UTC (permalink / raw)
To: 'Christophe Leroy', Geert Uytterhoeven
Cc: Krzysztof Kozlowski, Rich Felker, Jiri Slaby, Michael S. Tsirkin,
David Airlie, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, netdev, Paul Mackerras, Linux-Arch,
Dave Jiang, Yoshinori Sato, Helge Deller, Linux-sh list,
Alexey Brodkin, Ben Skeggs, nouveau, Dave Airlie, Matt Turner,
arcml, Nick Kossifidis, Allen Hubbe, Arnd Bergmann, alpha,
Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, Vineet Gupta,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Daniel Vetter, Jon Mason, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
From: Christophe Leroy
> Sent: 08 January 2020 08:49
...
> And as pointed by Arnd, the volatile is really only necessary for the
> dereference itself, should the arch use dereferencing.
I've had trouble with some versions of gcc and reading of 'volatile unsigned char *'.
It tended to follow the memory read with an extra mask with 0xff.
(I suspect that internally the value landed into a temporary 'int' variable.)
I got better code using memory barriers.
So putting an asm barrier for the exact location of the memory read
either side of the read should have the desired effect without adding
extra instructions.
(You might think 'volatile' would mean that - but it doesn't.)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 77+ messages in thread
* RE: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 17:39 ` David Laight
0 siblings, 0 replies; 77+ messages in thread
From: David Laight @ 2020-01-08 17:39 UTC (permalink / raw)
To: 'Christophe Leroy', Geert Uytterhoeven
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Krzysztof Kozlowski, Ben Skeggs, Dave Airlie, Matt Turner, arcml,
Nick Kossifidis, Allen Hubbe, Arnd Bergmann, Jon Mason,
Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, netdev,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Vineet Gupta, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
From: Christophe Leroy
> Sent: 08 January 2020 08:49
...
> And as pointed by Arnd, the volatile is really only necessary for the
> dereference itself, should the arch use dereferencing.
I've had trouble with some versions of gcc and reading of 'volatile unsigned char *'.
It tended to follow the memory read with an extra mask with 0xff.
(I suspect that internally the value landed into a temporary 'int' variable.)
I got better code using memory barriers.
So putting an asm barrier for the exact location of the memory read
either side of the read should have the desired effect without adding
extra instructions.
(You might think 'volatile' would mean that - but it doesn't.)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread
* RE: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 17:39 ` David Laight
0 siblings, 0 replies; 77+ messages in thread
From: David Laight @ 2020-01-08 17:39 UTC (permalink / raw)
To: 'Christophe Leroy', Geert Uytterhoeven
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Krzysztof Kozlowski, Ben Skeggs, Dave Airlie, Matt Turner, arcml,
Nick Kossifidis, Allen Hubbe, Arnd Bergmann, Jon Mason,
Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, netdev,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Vineet Gupta, Daniel Vetter, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
From: Christophe Leroy
> Sent: 08 January 2020 08:49
...
> And as pointed by Arnd, the volatile is really only necessary for the
> dereference itself, should the arch use dereferencing.
I've had trouble with some versions of gcc and reading of 'volatile unsigned char *'.
It tended to follow the memory read with an extra mask with 0xff.
(I suspect that internally the value landed into a temporary 'int' variable.)
I got better code using memory barriers.
So putting an asm barrier for the exact location of the memory read
either side of the read should have the desired effect without adding
extra instructions.
(You might think 'volatile' would mean that - but it doesn't.)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
^ permalink raw reply [flat|nested] 77+ messages in thread
* RE: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 17:39 ` David Laight
0 siblings, 0 replies; 77+ messages in thread
From: David Laight @ 2020-01-08 17:39 UTC (permalink / raw)
To: 'Christophe Leroy', Geert Uytterhoeven
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Krzysztof Kozlowski, Ben Skeggs, Dave Airlie, Matt Turner, arcml,
Nick Kossifidis, Allen Hubbe, Arnd Bergmann, Jon Mason,
Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, netdev,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Vineet Gupta, Daniel Vetter, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
From: Christophe Leroy
> Sent: 08 January 2020 08:49
...
> And as pointed by Arnd, the volatile is really only necessary for the
> dereference itself, should the arch use dereferencing.
I've had trouble with some versions of gcc and reading of 'volatile unsigned char *'.
It tended to follow the memory read with an extra mask with 0xff.
(I suspect that internally the value landed into a temporary 'int' variable.)
I got better code using memory barriers.
So putting an asm barrier for the exact location of the memory read
either side of the read should have the desired effect without adding
extra instructions.
(You might think 'volatile' would mean that - but it doesn't.)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
2020-01-08 8:35 ` Christophe Leroy
` (6 preceding siblings ...)
(?)
@ 2020-01-08 8:44 ` Arnd Bergmann
-1 siblings, 0 replies; 77+ messages in thread
From: Arnd Bergmann @ 2020-01-08 8:44 UTC (permalink / raw)
To: Christophe Leroy
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
ML nouveau, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Krzysztof Kozlowski, Geert Uytterhoeven, Ben Skeggs, Dave Airlie,
Matt Turner, arcml, Nick Kossifidis, Allen
On Wed, Jan 8, 2020 at 9:36 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > I'll add to this one also changes to ioreadX_rep() and add another
> > patch for volatile for reads and writes. I guess your review will be
> > appreciated once more because of ioreadX_rep()
> >
>
> volatile should really only be used where deemed necessary:
>
> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>
> It is said: " ... accessor functions might use volatile on
> architectures where direct I/O memory access does work. Essentially,
> each accessor call becomes a little critical section on its own and
> ensures that the access happens as expected by the programmer."
The I/O accessors are one of the few places in which 'volatile' generally
makes sense, at least for the implementations that do a plain pointer
dereference (probably none of the ones in question here).
In case of readl/writel, this is what we do in asm-generic:
static inline u32 __raw_readl(const volatile void __iomem *addr)
{
return *(const volatile u32 __force *)addr;
}
The __force-cast that removes the __iomem here also means that
the 'volatile' keyword could be dropped from the argument list,
as it has no real effect any more, but then there are a few drivers
that mark their iomem pointers as either 'volatile void __iomem*' or
(worse) 'volatile void *', so we keep it in the argument list to not
add warnings for those drivers.
It may be time to change these drivers to not use volatile for __iomem
pointers, but that seems out of scope for what Krzysztof is trying
to do. Ideally we would be consistent here though, either using volatile
all the time or never.
Arnd
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
2020-01-08 8:35 ` Christophe Leroy
` (3 preceding siblings ...)
(?)
@ 2020-01-08 8:44 ` Arnd Bergmann
-1 siblings, 0 replies; 77+ messages in thread
From: Arnd Bergmann @ 2020-01-08 8:44 UTC (permalink / raw)
To: Christophe Leroy
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
ML nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Krzysztof Kozlowski, Geert Uytterhoeven, Ben Skeggs, Dave Airlie,
Matt Turner, arcml, Nick Kossifidis, Allen Hubbe, Jon Mason,
Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, netdev,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Vineet Gupta, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
On Wed, Jan 8, 2020 at 9:36 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > I'll add to this one also changes to ioreadX_rep() and add another
> > patch for volatile for reads and writes. I guess your review will be
> > appreciated once more because of ioreadX_rep()
> >
>
> volatile should really only be used where deemed necessary:
>
> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>
> It is said: " ... accessor functions might use volatile on
> architectures where direct I/O memory access does work. Essentially,
> each accessor call becomes a little critical section on its own and
> ensures that the access happens as expected by the programmer."
The I/O accessors are one of the few places in which 'volatile' generally
makes sense, at least for the implementations that do a plain pointer
dereference (probably none of the ones in question here).
In case of readl/writel, this is what we do in asm-generic:
static inline u32 __raw_readl(const volatile void __iomem *addr)
{
return *(const volatile u32 __force *)addr;
}
The __force-cast that removes the __iomem here also means that
the 'volatile' keyword could be dropped from the argument list,
as it has no real effect any more, but then there are a few drivers
that mark their iomem pointers as either 'volatile void __iomem*' or
(worse) 'volatile void *', so we keep it in the argument list to not
add warnings for those drivers.
It may be time to change these drivers to not use volatile for __iomem
pointers, but that seems out of scope for what Krzysztof is trying
to do. Ideally we would be consistent here though, either using volatile
all the time or never.
Arnd
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:44 ` Arnd Bergmann
0 siblings, 0 replies; 77+ messages in thread
From: Arnd Bergmann @ 2020-01-08 8:44 UTC (permalink / raw)
To: Christophe Leroy
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
ML nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Krzysztof Kozlowski, Geert Uytterhoeven, Ben Skeggs, Dave Airlie,
Matt Turner, arcml, Nick Kossifidis, Allen Hubbe, Jon Mason,
Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, netdev,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Vineet Gupta, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
On Wed, Jan 8, 2020 at 9:36 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > I'll add to this one also changes to ioreadX_rep() and add another
> > patch for volatile for reads and writes. I guess your review will be
> > appreciated once more because of ioreadX_rep()
> >
>
> volatile should really only be used where deemed necessary:
>
> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>
> It is said: " ... accessor functions might use volatile on
> architectures where direct I/O memory access does work. Essentially,
> each accessor call becomes a little critical section on its own and
> ensures that the access happens as expected by the programmer."
The I/O accessors are one of the few places in which 'volatile' generally
makes sense, at least for the implementations that do a plain pointer
dereference (probably none of the ones in question here).
In case of readl/writel, this is what we do in asm-generic:
static inline u32 __raw_readl(const volatile void __iomem *addr)
{
return *(const volatile u32 __force *)addr;
}
The __force-cast that removes the __iomem here also means that
the 'volatile' keyword could be dropped from the argument list,
as it has no real effect any more, but then there are a few drivers
that mark their iomem pointers as either 'volatile void __iomem*' or
(worse) 'volatile void *', so we keep it in the argument list to not
add warnings for those drivers.
It may be time to change these drivers to not use volatile for __iomem
pointers, but that seems out of scope for what Krzysztof is trying
to do. Ideally we would be consistent here though, either using volatile
all the time or never.
Arnd
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:44 ` Arnd Bergmann
0 siblings, 0 replies; 77+ messages in thread
From: Arnd Bergmann @ 2020-01-08 8:44 UTC (permalink / raw)
To: Christophe Leroy
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
ML nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Krzysztof Kozlowski, Geert Uytterhoeven, Ben Skeggs, Dave Airlie,
Matt Turner, arcml, Nick Kossifidis, Allen Hubbe, Jon Mason,
Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, netdev,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Vineet Gupta, Daniel Vetter, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
On Wed, Jan 8, 2020 at 9:36 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > I'll add to this one also changes to ioreadX_rep() and add another
> > patch for volatile for reads and writes. I guess your review will be
> > appreciated once more because of ioreadX_rep()
> >
>
> volatile should really only be used where deemed necessary:
>
> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>
> It is said: " ... accessor functions might use volatile on
> architectures where direct I/O memory access does work. Essentially,
> each accessor call becomes a little critical section on its own and
> ensures that the access happens as expected by the programmer."
The I/O accessors are one of the few places in which 'volatile' generally
makes sense, at least for the implementations that do a plain pointer
dereference (probably none of the ones in question here).
In case of readl/writel, this is what we do in asm-generic:
static inline u32 __raw_readl(const volatile void __iomem *addr)
{
return *(const volatile u32 __force *)addr;
}
The __force-cast that removes the __iomem here also means that
the 'volatile' keyword could be dropped from the argument list,
as it has no real effect any more, but then there are a few drivers
that mark their iomem pointers as either 'volatile void __iomem*' or
(worse) 'volatile void *', so we keep it in the argument list to not
add warnings for those drivers.
It may be time to change these drivers to not use volatile for __iomem
pointers, but that seems out of scope for what Krzysztof is trying
to do. Ideally we would be consistent here though, either using volatile
all the time or never.
Arnd
_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:44 ` Arnd Bergmann
0 siblings, 0 replies; 77+ messages in thread
From: Arnd Bergmann @ 2020-01-08 8:44 UTC (permalink / raw)
To: Christophe Leroy
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
ML nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Krzysztof Kozlowski, Geert Uytterhoeven, Ben Skeggs, Dave Airlie,
Matt Turner, arcml, Nick Kossifidis, Allen Hubbe, Jon Mason,
Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, netdev,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Vineet Gupta, Daniel Vetter, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
On Wed, Jan 8, 2020 at 9:36 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > I'll add to this one also changes to ioreadX_rep() and add another
> > patch for volatile for reads and writes. I guess your review will be
> > appreciated once more because of ioreadX_rep()
> >
>
> volatile should really only be used where deemed necessary:
>
> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>
> It is said: " ... accessor functions might use volatile on
> architectures where direct I/O memory access does work. Essentially,
> each accessor call becomes a little critical section on its own and
> ensures that the access happens as expected by the programmer."
The I/O accessors are one of the few places in which 'volatile' generally
makes sense, at least for the implementations that do a plain pointer
dereference (probably none of the ones in question here).
In case of readl/writel, this is what we do in asm-generic:
static inline u32 __raw_readl(const volatile void __iomem *addr)
{
return *(const volatile u32 __force *)addr;
}
The __force-cast that removes the __iomem here also means that
the 'volatile' keyword could be dropped from the argument list,
as it has no real effect any more, but then there are a few drivers
that mark their iomem pointers as either 'volatile void __iomem*' or
(worse) 'volatile void *', so we keep it in the argument list to not
add warnings for those drivers.
It may be time to change these drivers to not use volatile for __iomem
pointers, but that seems out of scope for what Krzysztof is trying
to do. Ideally we would be consistent here though, either using volatile
all the time or never.
Arnd
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:44 ` Arnd Bergmann
0 siblings, 0 replies; 77+ messages in thread
From: Arnd Bergmann @ 2020-01-08 8:44 UTC (permalink / raw)
To: Christophe Leroy
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
ML nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Krzysztof Kozlowski, Geert Uytterhoeven, Ben Skeggs, Dave Airlie,
Matt Turner, arcml, Nick
On Wed, Jan 8, 2020 at 9:36 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > I'll add to this one also changes to ioreadX_rep() and add another
> > patch for volatile for reads and writes. I guess your review will be
> > appreciated once more because of ioreadX_rep()
> >
>
> volatile should really only be used where deemed necessary:
>
> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>
> It is said: " ... accessor functions might use volatile on
> architectures where direct I/O memory access does work. Essentially,
> each accessor call becomes a little critical section on its own and
> ensures that the access happens as expected by the programmer."
The I/O accessors are one of the few places in which 'volatile' generally
makes sense, at least for the implementations that do a plain pointer
dereference (probably none of the ones in question here).
In case of readl/writel, this is what we do in asm-generic:
static inline u32 __raw_readl(const volatile void __iomem *addr)
{
return *(const volatile u32 __force *)addr;
}
The __force-cast that removes the __iomem here also means that
the 'volatile' keyword could be dropped from the argument list,
as it has no real effect any more, but then there are a few drivers
that mark their iomem pointers as either 'volatile void __iomem*' or
(worse) 'volatile void *', so we keep it in the argument list to not
add warnings for those drivers.
It may be time to change these drivers to not use volatile for __iomem
pointers, but that seems out of scope for what Krzysztof is trying
to do. Ideally we would be consistent here though, either using volatile
all the time or never.
Arnd
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 8:44 ` Arnd Bergmann
0 siblings, 0 replies; 77+ messages in thread
From: Arnd Bergmann @ 2020-01-08 8:44 UTC (permalink / raw)
To: Christophe Leroy
Cc: Krzysztof Kozlowski, Geert Uytterhoeven, Rich Felker, Jiri Slaby,
Michael S. Tsirkin, David Airlie, Jason Wang, DRI Development,
virtualization, James E.J. Bottomley, netdev, Paul Mackerras,
Linux-Arch, Dave Jiang, Yoshinori Sato, Helge Deller,
Linux-sh list, Alexey Brodkin, Ben Skeggs, ML nouveau,
Dave Airlie, Matt Turner, arcml, Nick Kossifidis, Allen Hubbe,
alpha, Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, Vineet Gupta,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Daniel Vetter, Jon Mason, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
On Wed, Jan 8, 2020 at 9:36 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > I'll add to this one also changes to ioreadX_rep() and add another
> > patch for volatile for reads and writes. I guess your review will be
> > appreciated once more because of ioreadX_rep()
> >
>
> volatile should really only be used where deemed necessary:
>
> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>
> It is said: " ... accessor functions might use volatile on
> architectures where direct I/O memory access does work. Essentially,
> each accessor call becomes a little critical section on its own and
> ensures that the access happens as expected by the programmer."
The I/O accessors are one of the few places in which 'volatile' generally
makes sense, at least for the implementations that do a plain pointer
dereference (probably none of the ones in question here).
In case of readl/writel, this is what we do in asm-generic:
static inline u32 __raw_readl(const volatile void __iomem *addr)
{
return *(const volatile u32 __force *)addr;
}
The __force-cast that removes the __iomem here also means that
the 'volatile' keyword could be dropped from the argument list,
as it has no real effect any more, but then there are a few drivers
that mark their iomem pointers as either 'volatile void __iomem*' or
(worse) 'volatile void *', so we keep it in the argument list to not
add warnings for those drivers.
It may be time to change these drivers to not use volatile for __iomem
pointers, but that seems out of scope for what Krzysztof is trying
to do. Ideally we would be consistent here though, either using volatile
all the time or never.
Arnd
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
2020-01-08 8:44 ` Arnd Bergmann
` (2 preceding siblings ...)
(?)
@ 2020-01-08 9:15 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 77+ messages in thread
From: Krzysztof Kozlowski @ 2020-01-08 9:15 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
ML nouveau, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Geert Uytterhoeven, Ben Skeggs, Dave Airlie, Matt Turner, arcml,
Nick Kossifidis, Allen Hubbe, Jon Mason
On Wed, Jan 08, 2020 at 09:44:36AM +0100, Arnd Bergmann wrote:
> On Wed, Jan 8, 2020 at 9:36 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> > Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > I'll add to this one also changes to ioreadX_rep() and add another
> > > patch for volatile for reads and writes. I guess your review will be
> > > appreciated once more because of ioreadX_rep()
> > >
> >
> > volatile should really only be used where deemed necessary:
> >
> > https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
> >
> > It is said: " ... accessor functions might use volatile on
> > architectures where direct I/O memory access does work. Essentially,
> > each accessor call becomes a little critical section on its own and
> > ensures that the access happens as expected by the programmer."
>
> The I/O accessors are one of the few places in which 'volatile' generally
> makes sense, at least for the implementations that do a plain pointer
> dereference (probably none of the ones in question here).
>
> In case of readl/writel, this is what we do in asm-generic:
>
> static inline u32 __raw_readl(const volatile void __iomem *addr)
> {
> return *(const volatile u32 __force *)addr;
> }
SuperH is another example:
1. ioread8_rep(void __iomem *addr, void *dst, unsigned long count)
calls mmio_insb()
2. static inline void mmio_insb(void __iomem *addr, u8 *dst, int count)
calls __raw_readb()
3. #define __raw_readb(a) (__chk_io_ptr(a), *(volatile u8 __force *)(a))
Even if interface was not marked as volatile, in fact its implementation
was casting to volatile.
> The __force-cast that removes the __iomem here also means that
> the 'volatile' keyword could be dropped from the argument list,
> as it has no real effect any more, but then there are a few drivers
> that mark their iomem pointers as either 'volatile void __iomem*' or
> (worse) 'volatile void *', so we keep it in the argument list to not
> add warnings for those drivers.
>
> It may be time to change these drivers to not use volatile for __iomem
> pointers, but that seems out of scope for what Krzysztof is trying
> to do. Ideally we would be consistent here though, either using volatile
> all the time or never.
Indeed. I guess there are no objections around const so let me send v2
for const only.
Best regards,
Krzysztof
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 9:15 ` Krzysztof Kozlowski
0 siblings, 0 replies; 77+ messages in thread
From: Krzysztof Kozlowski @ 2020-01-08 9:15 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Christophe Leroy, Geert Uytterhoeven, Rich Felker, Jiri Slaby,
Michael S. Tsirkin, David Airlie, Jason Wang, DRI Development,
virtualization, James E.J. Bottomley, netdev, Paul Mackerras,
Linux-Arch, Dave Jiang, Yoshinori Sato, Helge Deller,
Linux-sh list, Alexey Brodkin, Ben Skeggs, ML nouveau,
Dave Airlie, Matt Turner, arcml, Nick Kossifidis, Allen Hubbe,
alpha, Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, Vineet Gupta,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Daniel Vetter, Jon Mason, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
On Wed, Jan 08, 2020 at 09:44:36AM +0100, Arnd Bergmann wrote:
> On Wed, Jan 8, 2020 at 9:36 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> > Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > I'll add to this one also changes to ioreadX_rep() and add another
> > > patch for volatile for reads and writes. I guess your review will be
> > > appreciated once more because of ioreadX_rep()
> > >
> >
> > volatile should really only be used where deemed necessary:
> >
> > https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
> >
> > It is said: " ... accessor functions might use volatile on
> > architectures where direct I/O memory access does work. Essentially,
> > each accessor call becomes a little critical section on its own and
> > ensures that the access happens as expected by the programmer."
>
> The I/O accessors are one of the few places in which 'volatile' generally
> makes sense, at least for the implementations that do a plain pointer
> dereference (probably none of the ones in question here).
>
> In case of readl/writel, this is what we do in asm-generic:
>
> static inline u32 __raw_readl(const volatile void __iomem *addr)
> {
> return *(const volatile u32 __force *)addr;
> }
SuperH is another example:
1. ioread8_rep(void __iomem *addr, void *dst, unsigned long count)
calls mmio_insb()
2. static inline void mmio_insb(void __iomem *addr, u8 *dst, int count)
calls __raw_readb()
3. #define __raw_readb(a) (__chk_io_ptr(a), *(volatile u8 __force *)(a))
Even if interface was not marked as volatile, in fact its implementation
was casting to volatile.
> The __force-cast that removes the __iomem here also means that
> the 'volatile' keyword could be dropped from the argument list,
> as it has no real effect any more, but then there are a few drivers
> that mark their iomem pointers as either 'volatile void __iomem*' or
> (worse) 'volatile void *', so we keep it in the argument list to not
> add warnings for those drivers.
>
> It may be time to change these drivers to not use volatile for __iomem
> pointers, but that seems out of scope for what Krzysztof is trying
> to do. Ideally we would be consistent here though, either using volatile
> all the time or never.
Indeed. I guess there are no objections around const so let me send v2
for const only.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 9:15 ` Krzysztof Kozlowski
0 siblings, 0 replies; 77+ messages in thread
From: Krzysztof Kozlowski @ 2020-01-08 9:15 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
ML nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Geert Uytterhoeven, Ben Skeggs, Dave Airlie, Matt Turner, arcml,
Nick Kossifidis, Allen Hubbe, Jon Mason, Ivan Kokshaysky,
Thomas Gleixner, Mauro Carvalho Chehab, Kalle Valo,
Richard Henderson, Christophe Leroy, Parisc List, netdev,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Vineet Gupta, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
On Wed, Jan 08, 2020 at 09:44:36AM +0100, Arnd Bergmann wrote:
> On Wed, Jan 8, 2020 at 9:36 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> > Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > I'll add to this one also changes to ioreadX_rep() and add another
> > > patch for volatile for reads and writes. I guess your review will be
> > > appreciated once more because of ioreadX_rep()
> > >
> >
> > volatile should really only be used where deemed necessary:
> >
> > https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
> >
> > It is said: " ... accessor functions might use volatile on
> > architectures where direct I/O memory access does work. Essentially,
> > each accessor call becomes a little critical section on its own and
> > ensures that the access happens as expected by the programmer."
>
> The I/O accessors are one of the few places in which 'volatile' generally
> makes sense, at least for the implementations that do a plain pointer
> dereference (probably none of the ones in question here).
>
> In case of readl/writel, this is what we do in asm-generic:
>
> static inline u32 __raw_readl(const volatile void __iomem *addr)
> {
> return *(const volatile u32 __force *)addr;
> }
SuperH is another example:
1. ioread8_rep(void __iomem *addr, void *dst, unsigned long count)
calls mmio_insb()
2. static inline void mmio_insb(void __iomem *addr, u8 *dst, int count)
calls __raw_readb()
3. #define __raw_readb(a) (__chk_io_ptr(a), *(volatile u8 __force *)(a))
Even if interface was not marked as volatile, in fact its implementation
was casting to volatile.
> The __force-cast that removes the __iomem here also means that
> the 'volatile' keyword could be dropped from the argument list,
> as it has no real effect any more, but then there are a few drivers
> that mark their iomem pointers as either 'volatile void __iomem*' or
> (worse) 'volatile void *', so we keep it in the argument list to not
> add warnings for those drivers.
>
> It may be time to change these drivers to not use volatile for __iomem
> pointers, but that seems out of scope for what Krzysztof is trying
> to do. Ideally we would be consistent here though, either using volatile
> all the time or never.
Indeed. I guess there are no objections around const so let me send v2
for const only.
Best regards,
Krzysztof
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 9:15 ` Krzysztof Kozlowski
0 siblings, 0 replies; 77+ messages in thread
From: Krzysztof Kozlowski @ 2020-01-08 9:15 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
ML nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Geert Uytterhoeven, Ben Skeggs, Dave Airlie, Matt Turner, arcml,
Nick Kossifidis, Allen Hubbe, Jon Mason, Ivan Kokshaysky,
Thomas Gleixner, Mauro Carvalho Chehab, Kalle Valo,
Richard Henderson, Christophe Leroy, Parisc List, netdev,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Vineet Gupta, Daniel Vetter, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
On Wed, Jan 08, 2020 at 09:44:36AM +0100, Arnd Bergmann wrote:
> On Wed, Jan 8, 2020 at 9:36 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> > Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > I'll add to this one also changes to ioreadX_rep() and add another
> > > patch for volatile for reads and writes. I guess your review will be
> > > appreciated once more because of ioreadX_rep()
> > >
> >
> > volatile should really only be used where deemed necessary:
> >
> > https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
> >
> > It is said: " ... accessor functions might use volatile on
> > architectures where direct I/O memory access does work. Essentially,
> > each accessor call becomes a little critical section on its own and
> > ensures that the access happens as expected by the programmer."
>
> The I/O accessors are one of the few places in which 'volatile' generally
> makes sense, at least for the implementations that do a plain pointer
> dereference (probably none of the ones in question here).
>
> In case of readl/writel, this is what we do in asm-generic:
>
> static inline u32 __raw_readl(const volatile void __iomem *addr)
> {
> return *(const volatile u32 __force *)addr;
> }
SuperH is another example:
1. ioread8_rep(void __iomem *addr, void *dst, unsigned long count)
calls mmio_insb()
2. static inline void mmio_insb(void __iomem *addr, u8 *dst, int count)
calls __raw_readb()
3. #define __raw_readb(a) (__chk_io_ptr(a), *(volatile u8 __force *)(a))
Even if interface was not marked as volatile, in fact its implementation
was casting to volatile.
> The __force-cast that removes the __iomem here also means that
> the 'volatile' keyword could be dropped from the argument list,
> as it has no real effect any more, but then there are a few drivers
> that mark their iomem pointers as either 'volatile void __iomem*' or
> (worse) 'volatile void *', so we keep it in the argument list to not
> add warnings for those drivers.
>
> It may be time to change these drivers to not use volatile for __iomem
> pointers, but that seems out of scope for what Krzysztof is trying
> to do. Ideally we would be consistent here though, either using volatile
> all the time or never.
Indeed. I guess there are no objections around const so let me send v2
for const only.
Best regards,
Krzysztof
_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 9:15 ` Krzysztof Kozlowski
0 siblings, 0 replies; 77+ messages in thread
From: Krzysztof Kozlowski @ 2020-01-08 9:15 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
ML nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Geert Uytterhoeven, Ben Skeggs, Dave Airlie, Matt Turner, arcml,
Nick Kossifidis, Allen Hubbe, Jon Mason, Ivan Kokshaysky,
Thomas Gleixner, Mauro Carvalho Chehab, Kalle Valo,
Richard Henderson, Parisc List, netdev, linux-wireless,
Linux Kernel Mailing List, Luis Chamberlain, Vineet Gupta,
Daniel Vetter, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
On Wed, Jan 08, 2020 at 09:44:36AM +0100, Arnd Bergmann wrote:
> On Wed, Jan 8, 2020 at 9:36 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> > Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > I'll add to this one also changes to ioreadX_rep() and add another
> > > patch for volatile for reads and writes. I guess your review will be
> > > appreciated once more because of ioreadX_rep()
> > >
> >
> > volatile should really only be used where deemed necessary:
> >
> > https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
> >
> > It is said: " ... accessor functions might use volatile on
> > architectures where direct I/O memory access does work. Essentially,
> > each accessor call becomes a little critical section on its own and
> > ensures that the access happens as expected by the programmer."
>
> The I/O accessors are one of the few places in which 'volatile' generally
> makes sense, at least for the implementations that do a plain pointer
> dereference (probably none of the ones in question here).
>
> In case of readl/writel, this is what we do in asm-generic:
>
> static inline u32 __raw_readl(const volatile void __iomem *addr)
> {
> return *(const volatile u32 __force *)addr;
> }
SuperH is another example:
1. ioread8_rep(void __iomem *addr, void *dst, unsigned long count)
calls mmio_insb()
2. static inline void mmio_insb(void __iomem *addr, u8 *dst, int count)
calls __raw_readb()
3. #define __raw_readb(a) (__chk_io_ptr(a), *(volatile u8 __force *)(a))
Even if interface was not marked as volatile, in fact its implementation
was casting to volatile.
> The __force-cast that removes the __iomem here also means that
> the 'volatile' keyword could be dropped from the argument list,
> as it has no real effect any more, but then there are a few drivers
> that mark their iomem pointers as either 'volatile void __iomem*' or
> (worse) 'volatile void *', so we keep it in the argument list to not
> add warnings for those drivers.
>
> It may be time to change these drivers to not use volatile for __iomem
> pointers, but that seems out of scope for what Krzysztof is trying
> to do. Ideally we would be consistent here though, either using volatile
> all the time or never.
Indeed. I guess there are no objections around const so let me send v2
for const only.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
2020-01-08 9:15 ` Krzysztof Kozlowski
` (3 preceding siblings ...)
(?)
@ 2020-01-08 11:55 ` Arnd Bergmann
-1 siblings, 0 replies; 77+ messages in thread
From: Arnd Bergmann @ 2020-01-08 11:55 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
ML nouveau, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Geert Uytterhoeven, Ben Skeggs, Dave Airlie, Matt Turner, arcml,
Nick Kossifidis, Allen Hubbe, Jon Mason
On Wed, Jan 8, 2020 at 10:15 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > The __force-cast that removes the __iomem here also means that
> > the 'volatile' keyword could be dropped from the argument list,
> > as it has no real effect any more, but then there are a few drivers
> > that mark their iomem pointers as either 'volatile void __iomem*' or
> > (worse) 'volatile void *', so we keep it in the argument list to not
> > add warnings for those drivers.
> >
> > It may be time to change these drivers to not use volatile for __iomem
> > pointers, but that seems out of scope for what Krzysztof is trying
> > to do. Ideally we would be consistent here though, either using volatile
> > all the time or never.
>
> Indeed. I guess there are no objections around const so let me send v2
> for const only.
Ok, sounds good. Maybe mention in the changelog then that the
'volatile' in the interface is intentionally left out, and that only users
of readl/writel still have it to deal with existing drivers.
Arnd
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
2020-01-08 9:15 ` Krzysztof Kozlowski
` (3 preceding siblings ...)
(?)
@ 2020-01-08 11:55 ` Arnd Bergmann
-1 siblings, 0 replies; 77+ messages in thread
From: Arnd Bergmann @ 2020-01-08 11:55 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
ML nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Geert Uytterhoeven, Ben Skeggs, Dave Airlie, Matt Turner, arcml,
Nick Kossifidis
On Wed, Jan 8, 2020 at 10:15 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > The __force-cast that removes the __iomem here also means that
> > the 'volatile' keyword could be dropped from the argument list,
> > as it has no real effect any more, but then there are a few drivers
> > that mark their iomem pointers as either 'volatile void __iomem*' or
> > (worse) 'volatile void *', so we keep it in the argument list to not
> > add warnings for those drivers.
> >
> > It may be time to change these drivers to not use volatile for __iomem
> > pointers, but that seems out of scope for what Krzysztof is trying
> > to do. Ideally we would be consistent here though, either using volatile
> > all the time or never.
>
> Indeed. I guess there are no objections around const so let me send v2
> for const only.
Ok, sounds good. Maybe mention in the changelog then that the
'volatile' in the interface is intentionally left out, and that only users
of readl/writel still have it to deal with existing drivers.
Arnd
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 11:55 ` Arnd Bergmann
0 siblings, 0 replies; 77+ messages in thread
From: Arnd Bergmann @ 2020-01-08 11:55 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
ML nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Geert Uytterhoeven, Ben Skeggs, Dave Airlie, Matt Turner, arcml,
Nick Kossifidis, Allen Hubbe
On Wed, Jan 8, 2020 at 10:15 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > The __force-cast that removes the __iomem here also means that
> > the 'volatile' keyword could be dropped from the argument list,
> > as it has no real effect any more, but then there are a few drivers
> > that mark their iomem pointers as either 'volatile void __iomem*' or
> > (worse) 'volatile void *', so we keep it in the argument list to not
> > add warnings for those drivers.
> >
> > It may be time to change these drivers to not use volatile for __iomem
> > pointers, but that seems out of scope for what Krzysztof is trying
> > to do. Ideally we would be consistent here though, either using volatile
> > all the time or never.
>
> Indeed. I guess there are no objections around const so let me send v2
> for const only.
Ok, sounds good. Maybe mention in the changelog then that the
'volatile' in the interface is intentionally left out, and that only users
of readl/writel still have it to deal with existing drivers.
Arnd
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 11:55 ` Arnd Bergmann
0 siblings, 0 replies; 77+ messages in thread
From: Arnd Bergmann @ 2020-01-08 11:55 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Christophe Leroy, Geert Uytterhoeven, Rich Felker, Jiri Slaby,
Michael S. Tsirkin, David Airlie, Jason Wang, DRI Development,
virtualization, James E.J. Bottomley, netdev, Paul Mackerras,
Linux-Arch, Dave Jiang, Yoshinori Sato, Helge Deller,
Linux-sh list, Alexey Brodkin, Ben Skeggs, ML nouveau,
Dave Airlie, Matt Turner, arcml, Nick Kossifidis, Allen Hubbe,
alpha, Ivan Kokshaysky, Thomas Gleixner, Mauro Carvalho Chehab,
Kalle Valo, Richard Henderson, Parisc List, Vineet Gupta,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Daniel Vetter, Jon Mason, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
On Wed, Jan 8, 2020 at 10:15 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > The __force-cast that removes the __iomem here also means that
> > the 'volatile' keyword could be dropped from the argument list,
> > as it has no real effect any more, but then there are a few drivers
> > that mark their iomem pointers as either 'volatile void __iomem*' or
> > (worse) 'volatile void *', so we keep it in the argument list to not
> > add warnings for those drivers.
> >
> > It may be time to change these drivers to not use volatile for __iomem
> > pointers, but that seems out of scope for what Krzysztof is trying
> > to do. Ideally we would be consistent here though, either using volatile
> > all the time or never.
>
> Indeed. I guess there are no objections around const so let me send v2
> for const only.
Ok, sounds good. Maybe mention in the changelog then that the
'volatile' in the interface is intentionally left out, and that only users
of readl/writel still have it to deal with existing drivers.
Arnd
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 11:55 ` Arnd Bergmann
0 siblings, 0 replies; 77+ messages in thread
From: Arnd Bergmann @ 2020-01-08 11:55 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
ML nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Geert Uytterhoeven, Ben Skeggs, Dave Airlie, Matt Turner, arcml,
Nick Kossifidis, Allen Hubbe, Jon Mason, Ivan Kokshaysky,
Thomas Gleixner, Mauro Carvalho Chehab, Kalle Valo,
Richard Henderson, Christophe Leroy, Parisc List, netdev,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Vineet Gupta, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
On Wed, Jan 8, 2020 at 10:15 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > The __force-cast that removes the __iomem here also means that
> > the 'volatile' keyword could be dropped from the argument list,
> > as it has no real effect any more, but then there are a few drivers
> > that mark their iomem pointers as either 'volatile void __iomem*' or
> > (worse) 'volatile void *', so we keep it in the argument list to not
> > add warnings for those drivers.
> >
> > It may be time to change these drivers to not use volatile for __iomem
> > pointers, but that seems out of scope for what Krzysztof is trying
> > to do. Ideally we would be consistent here though, either using volatile
> > all the time or never.
>
> Indeed. I guess there are no objections around const so let me send v2
> for const only.
Ok, sounds good. Maybe mention in the changelog then that the
'volatile' in the interface is intentionally left out, and that only users
of readl/writel still have it to deal with existing drivers.
Arnd
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 11:55 ` Arnd Bergmann
0 siblings, 0 replies; 77+ messages in thread
From: Arnd Bergmann @ 2020-01-08 11:55 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
ML nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Geert Uytterhoeven, Ben Skeggs, Dave Airlie, Matt Turner, arcml,
Nick Kossifidis, Allen Hubbe, Jon Mason, Ivan Kokshaysky,
Thomas Gleixner, Mauro Carvalho Chehab, Kalle Valo,
Richard Henderson, Christophe Leroy, Parisc List, netdev,
linux-wireless, Linux Kernel Mailing List, Luis Chamberlain,
Vineet Gupta, Daniel Vetter, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
On Wed, Jan 8, 2020 at 10:15 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > The __force-cast that removes the __iomem here also means that
> > the 'volatile' keyword could be dropped from the argument list,
> > as it has no real effect any more, but then there are a few drivers
> > that mark their iomem pointers as either 'volatile void __iomem*' or
> > (worse) 'volatile void *', so we keep it in the argument list to not
> > add warnings for those drivers.
> >
> > It may be time to change these drivers to not use volatile for __iomem
> > pointers, but that seems out of scope for what Krzysztof is trying
> > to do. Ideally we would be consistent here though, either using volatile
> > all the time or never.
>
> Indeed. I guess there are no objections around const so let me send v2
> for const only.
Ok, sounds good. Maybe mention in the changelog then that the
'volatile' in the interface is intentionally left out, and that only users
of readl/writel still have it to deal with existing drivers.
Arnd
_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
@ 2020-01-08 11:55 ` Arnd Bergmann
0 siblings, 0 replies; 77+ messages in thread
From: Arnd Bergmann @ 2020-01-08 11:55 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rich Felker, Jiri Slaby, Michael S. Tsirkin, David Airlie,
ML nouveau, Jason Wang, DRI Development, virtualization,
James E.J. Bottomley, Paul Mackerras, Linux-Arch, Dave Jiang,
Yoshinori Sato, Helge Deller, Linux-sh list, Alexey Brodkin,
Geert Uytterhoeven, Ben Skeggs, Dave Airlie, Matt Turner, arcml,
Nick Kossifidis, Allen Hubbe, Jon Mason, Ivan Kokshaysky,
Thomas Gleixner, Mauro Carvalho Chehab, Kalle Valo,
Richard Henderson, Parisc List, netdev, linux-wireless,
Linux Kernel Mailing List, Luis Chamberlain, Vineet Gupta,
Daniel Vetter, alpha, linux-ntb, Andrew Morton,
Linux Media Mailing List, linuxppc-dev, David S. Miller
On Wed, Jan 8, 2020 at 10:15 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > The __force-cast that removes the __iomem here also means that
> > the 'volatile' keyword could be dropped from the argument list,
> > as it has no real effect any more, but then there are a few drivers
> > that mark their iomem pointers as either 'volatile void __iomem*' or
> > (worse) 'volatile void *', so we keep it in the argument list to not
> > add warnings for those drivers.
> >
> > It may be time to change these drivers to not use volatile for __iomem
> > pointers, but that seems out of scope for what Krzysztof is trying
> > to do. Ideally we would be consistent here though, either using volatile
> > all the time or never.
>
> Indeed. I guess there are no objections around const so let me send v2
> for const only.
Ok, sounds good. Maybe mention in the changelog then that the
'volatile' in the interface is intentionally left out, and that only users
of readl/writel still have it to deal with existing drivers.
Arnd
^ permalink raw reply [flat|nested] 77+ messages in thread