Update the 2nd argument of _write_atomic() from 'unsigned long x'
to 'void *x' to allow write_atomic() to handle non-scalar types,
aligning it with read_atomic(), which can work with non-scalar types.
Additionally, update the implementation of _add_sized() to use
"writeX_cpu(readX_cpu(p) + x, p)" instead of
"write_atomic(ptr, read_atomic(ptr) + x)" because 'ptr' is defined
as 'volatile uintX_t *'.
This avoids a compilation error that occurs when passing the 2nd
argument to _write_atomic() (i.e., "passing argument 2 of '_write_atomic'
discards 'volatile' qualifier from pointer target type") since the 2nd
argument of _write_atomic() is now 'void *' instead of 'unsigned long'.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v6:
- new patch.
---
xen/arch/riscv/include/asm/atomic.h | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/xen/arch/riscv/include/asm/atomic.h b/xen/arch/riscv/include/asm/atomic.h
index 3c6bd86406..92b92fb4d4 100644
--- a/xen/arch/riscv/include/asm/atomic.h
+++ b/xen/arch/riscv/include/asm/atomic.h
@@ -54,16 +54,16 @@ static always_inline void read_atomic_size(const volatile void *p,
})
static always_inline void _write_atomic(volatile void *p,
- unsigned long x,
+ void *x,
unsigned int size)
{
switch ( size )
{
- case 1: writeb_cpu(x, p); break;
- case 2: writew_cpu(x, p); break;
- case 4: writel_cpu(x, p); break;
+ case 1: writeb_cpu(*(uint8_t *)x, p); break;
+ case 2: writew_cpu(*(uint16_t *)x, p); break;
+ case 4: writel_cpu(*(uint32_t *)x, p); break;
#ifndef CONFIG_RISCV_32
- case 8: writeq_cpu(x, p); break;
+ case 8: writeq_cpu(*(uint64_t *)x, p); break;
#endif
default: __bad_atomic_size(); break;
}
@@ -72,7 +72,7 @@ static always_inline void _write_atomic(volatile void *p,
#define write_atomic(p, x) \
({ \
typeof(*(p)) x_ = (x); \
- _write_atomic(p, x_, sizeof(*(p))); \
+ _write_atomic(p, &x_, sizeof(*(p))); \
})
static always_inline void _add_sized(volatile void *p,
@@ -82,27 +82,23 @@ static always_inline void _add_sized(volatile void *p,
{
case 1:
{
- volatile uint8_t *ptr = p;
- write_atomic(ptr, read_atomic(ptr) + x);
+ writeb_cpu(readb_cpu(p) + x, p);
break;
}
case 2:
{
- volatile uint16_t *ptr = p;
- write_atomic(ptr, read_atomic(ptr) + x);
+ writew_cpu(readw_cpu(p) + x, p);
break;
}
case 4:
{
- volatile uint32_t *ptr = p;
- write_atomic(ptr, read_atomic(ptr) + x);
+ writel_cpu(readl_cpu(p) + x, p);
break;
}
#ifndef CONFIG_RISCV_32
case 8:
{
- volatile uint64_t *ptr = p;
- write_atomic(ptr, read_atomic(ptr) + x);
+ writeq_cpu(readw_cpu(p) + x, p);
break;
}
#endif
--
2.46.0On 02.09.2024 19:01, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/atomic.h
> +++ b/xen/arch/riscv/include/asm/atomic.h
> @@ -54,16 +54,16 @@ static always_inline void read_atomic_size(const volatile void *p,
> })
>
> static always_inline void _write_atomic(volatile void *p,
> - unsigned long x,
> + void *x,
Pointer-to-const please, to further aid in easily recognizing which
parameter is what. After all ...
> unsigned int size)
> {
> switch ( size )
> {
> - case 1: writeb_cpu(x, p); break;
> - case 2: writew_cpu(x, p); break;
> - case 4: writel_cpu(x, p); break;
... unhelpfully enough parameters are then swapped, just to confuse
things.
> + case 1: writeb_cpu(*(uint8_t *)x, p); break;
> + case 2: writew_cpu(*(uint16_t *)x, p); break;
> + case 4: writel_cpu(*(uint32_t *)x, p); break;
> #ifndef CONFIG_RISCV_32
> - case 8: writeq_cpu(x, p); break;
> + case 8: writeq_cpu(*(uint64_t *)x, p); break;
With const added to the parameter, please further make sure to then not
cast that away again.
> @@ -72,7 +72,7 @@ static always_inline void _write_atomic(volatile void *p,
> #define write_atomic(p, x) \
> ({ \
> typeof(*(p)) x_ = (x); \
> - _write_atomic(p, x_, sizeof(*(p))); \
> + _write_atomic(p, &x_, sizeof(*(p))); \
> })
>
> static always_inline void _add_sized(volatile void *p,
> @@ -82,27 +82,23 @@ static always_inline void _add_sized(volatile void *p,
> {
> case 1:
> {
> - volatile uint8_t *ptr = p;
> - write_atomic(ptr, read_atomic(ptr) + x);
> + writeb_cpu(readb_cpu(p) + x, p);
> break;
> }
> case 2:
> {
> - volatile uint16_t *ptr = p;
> - write_atomic(ptr, read_atomic(ptr) + x);
> + writew_cpu(readw_cpu(p) + x, p);
> break;
> }
> case 4:
> {
> - volatile uint32_t *ptr = p;
> - write_atomic(ptr, read_atomic(ptr) + x);
> + writel_cpu(readl_cpu(p) + x, p);
> break;
> }
> #ifndef CONFIG_RISCV_32
> case 8:
> {
> - volatile uint64_t *ptr = p;
> - write_atomic(ptr, read_atomic(ptr) + x);
> + writeq_cpu(readw_cpu(p) + x, p);
> break;
> }
> #endif
I'm afraid I don't understand this part, or more specifically the respective
part of the description. It is the first parameter of write_atomic() which is
volatile qualified. And it is the first argument that's volatile qualified
here. Isn't the problem entirely unrelated to volatile-ness, and instead a
result of the other parameter changing from scalar to pointer type, which
doesn't fit the addition expressions you pass in?
Also you surely mean readq_cpu() in the 8-byte case.
Jan
On Tue, 2024-09-10 at 11:53 +0200, Jan Beulich wrote:
> On 02.09.2024 19:01, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/include/asm/atomic.h
> > +++ b/xen/arch/riscv/include/asm/atomic.h
> > @@ -54,16 +54,16 @@ static always_inline void
> > read_atomic_size(const volatile void *p,
> > })
> >
> > static always_inline void _write_atomic(volatile void *p,
> > - unsigned long x,
> > + void *x,
>
> Pointer-to-const please, to further aid in easily recognizing which
> parameter is what. After all ...
>
> > unsigned int size)
> > {
> > switch ( size )
> > {
> > - case 1: writeb_cpu(x, p); break;
> > - case 2: writew_cpu(x, p); break;
> > - case 4: writel_cpu(x, p); break;
>
> ... unhelpfully enough parameters are then swapped, just to confuse
> things.
If it would be better to keep 'unsigned long' as the type of x, then,
if I am not mistaken, write_atomic() should be updated in the following
way:
#define write_atomic(p, x) \
({ \
typeof(*(p)) x_ = (x); \
_write_atomic(p, *(unsigned long *)&x_, sizeof(*(p)));
\
})
However, I am not sure if it is safe when x is a 2-byte value (for
example) that it will read more than 2 bytes before passing the value
to the _write_atomic() function.
>
> > + case 1: writeb_cpu(*(uint8_t *)x, p); break;
> > + case 2: writew_cpu(*(uint16_t *)x, p); break;
> > + case 4: writel_cpu(*(uint32_t *)x, p); break;
> > #ifndef CONFIG_RISCV_32
> > - case 8: writeq_cpu(x, p); break;
> > + case 8: writeq_cpu(*(uint64_t *)x, p); break;
>
> With const added to the parameter, please further make sure to then
> not
> cast that away again.
>
> > @@ -72,7 +72,7 @@ static always_inline void _write_atomic(volatile
> > void *p,
> > #define write_atomic(p, x) \
> > ({ \
> > typeof(*(p)) x_ = (x); \
> > - _write_atomic(p, x_, sizeof(*(p))); \
> > + _write_atomic(p, &x_, sizeof(*(p))); \
> > })
> >
> > static always_inline void _add_sized(volatile void *p,
> > @@ -82,27 +82,23 @@ static always_inline void _add_sized(volatile
> > void *p,
> > {
> > case 1:
> > {
> > - volatile uint8_t *ptr = p;
> > - write_atomic(ptr, read_atomic(ptr) + x);
> > + writeb_cpu(readb_cpu(p) + x, p);
> > break;
> > }
> > case 2:
> > {
> > - volatile uint16_t *ptr = p;
> > - write_atomic(ptr, read_atomic(ptr) + x);
> > + writew_cpu(readw_cpu(p) + x, p);
> > break;
> > }
> > case 4:
> > {
> > - volatile uint32_t *ptr = p;
> > - write_atomic(ptr, read_atomic(ptr) + x);
> > + writel_cpu(readl_cpu(p) + x, p);
> > break;
> > }
> > #ifndef CONFIG_RISCV_32
> > case 8:
> > {
> > - volatile uint64_t *ptr = p;
> > - write_atomic(ptr, read_atomic(ptr) + x);
> > + writeq_cpu(readw_cpu(p) + x, p);
> > break;
> > }
> > #endif
>
> I'm afraid I don't understand this part, or more specifically the
> respective
> part of the description. It is the first parameter of write_atomic()
> which is
> volatile qualified. And it is the first argument that's volatile
> qualified
> here. Isn't the problem entirely unrelated to volatile-ness, and
> instead a
> result of the other parameter changing from scalar to pointer type,
> which
> doesn't fit the addition expressions you pass in?
if _add_sized() is defined as it was before:
static always_inline void _add_sized(volatile void *p,
unsigned long x, unsigned int
size)
{
switch ( size )
{
case 1:
{
volatile uint8_t *ptr = p;
write_atomic(ptr, read_atomic(ptr) + x);
break;
}
...
Then write_atomic(ptr, read_atomic(ptr) + x) will be be changed to:
volatile uint8_t x_ = (x);
And that will cause a compiler error:
./arch/riscv/include/asm/atomic.h:75:22: error: passing argument 2
of '_write_atomic' discards 'volatile' qualifier from pointer target
type [-Werror=discarded-qualifiers]
75 | _write_atomic(p, &x_, sizeof(*(p)));
Because it can't cast 'volatile uint8_t *' to 'void *':
expected 'void *' but argument is of type 'volatile uint8_t *' {aka
'volatile unsigned char *'}
>
> Also you surely mean readq_cpu() in the 8-byte case.
Yes, thanks for finding that.
~ Oleksii
On 10.09.2024 17:28, oleksii.kurochko@gmail.com wrote:
> On Tue, 2024-09-10 at 11:53 +0200, Jan Beulich wrote:
>> On 02.09.2024 19:01, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/include/asm/atomic.h
>>> +++ b/xen/arch/riscv/include/asm/atomic.h
>>> @@ -54,16 +54,16 @@ static always_inline void
>>> read_atomic_size(const volatile void *p,
>>> })
>>>
>>> static always_inline void _write_atomic(volatile void *p,
>>> - unsigned long x,
>>> + void *x,
>>
>> Pointer-to-const please, to further aid in easily recognizing which
>> parameter is what. After all ...
>>
>>> unsigned int size)
>>> {
>>> switch ( size )
>>> {
>>> - case 1: writeb_cpu(x, p); break;
>>> - case 2: writew_cpu(x, p); break;
>>> - case 4: writel_cpu(x, p); break;
>>
>> ... unhelpfully enough parameters are then swapped, just to confuse
>> things.
> If it would be better to keep 'unsigned long' as the type of x, then,
> if I am not mistaken, write_atomic() should be updated in the following
> way:
> #define write_atomic(p, x) \
> ({ \
> typeof(*(p)) x_ = (x); \
> _write_atomic(p, *(unsigned long *)&x_, sizeof(*(p)));
> \
> })
> However, I am not sure if it is safe when x is a 2-byte value (for
> example) that it will read more than 2 bytes before passing the value
> to the _write_atomic() function.
No, that's definitely unsafe.
>>> @@ -72,7 +72,7 @@ static always_inline void _write_atomic(volatile
>>> void *p,
>>> #define write_atomic(p, x) \
>>> ({ \
>>> typeof(*(p)) x_ = (x); \
>>> - _write_atomic(p, x_, sizeof(*(p))); \
>>> + _write_atomic(p, &x_, sizeof(*(p))); \
>>> })
>>>
>>> static always_inline void _add_sized(volatile void *p,
>>> @@ -82,27 +82,23 @@ static always_inline void _add_sized(volatile
>>> void *p,
>>> {
>>> case 1:
>>> {
>>> - volatile uint8_t *ptr = p;
>>> - write_atomic(ptr, read_atomic(ptr) + x);
>>> + writeb_cpu(readb_cpu(p) + x, p);
>>> break;
>>> }
>>> case 2:
>>> {
>>> - volatile uint16_t *ptr = p;
>>> - write_atomic(ptr, read_atomic(ptr) + x);
>>> + writew_cpu(readw_cpu(p) + x, p);
>>> break;
>>> }
>>> case 4:
>>> {
>>> - volatile uint32_t *ptr = p;
>>> - write_atomic(ptr, read_atomic(ptr) + x);
>>> + writel_cpu(readl_cpu(p) + x, p);
>>> break;
>>> }
>>> #ifndef CONFIG_RISCV_32
>>> case 8:
>>> {
>>> - volatile uint64_t *ptr = p;
>>> - write_atomic(ptr, read_atomic(ptr) + x);
>>> + writeq_cpu(readw_cpu(p) + x, p);
>>> break;
>>> }
>>> #endif
>>
>> I'm afraid I don't understand this part, or more specifically the
>> respective
>> part of the description. It is the first parameter of write_atomic()
>> which is
>> volatile qualified. And it is the first argument that's volatile
>> qualified
>> here. Isn't the problem entirely unrelated to volatile-ness, and
>> instead a
>> result of the other parameter changing from scalar to pointer type,
>> which
>> doesn't fit the addition expressions you pass in?
> if _add_sized() is defined as it was before:
> static always_inline void _add_sized(volatile void *p,
> unsigned long x, unsigned int
> size)
> {
> switch ( size )
> {
> case 1:
> {
> volatile uint8_t *ptr = p;
> write_atomic(ptr, read_atomic(ptr) + x);
> break;
> }
> ...
> Then write_atomic(ptr, read_atomic(ptr) + x) will be be changed to:
> volatile uint8_t x_ = (x);
>
> And that will cause a compiler error:
> ./arch/riscv/include/asm/atomic.h:75:22: error: passing argument 2
> of '_write_atomic' discards 'volatile' qualifier from pointer target
> type [-Werror=discarded-qualifiers]
> 75 | _write_atomic(p, &x_, sizeof(*(p)));
> Because it can't cast 'volatile uint8_t *' to 'void *':
> expected 'void *' but argument is of type 'volatile uint8_t *' {aka
> 'volatile unsigned char *'}
Oh, I think I see now. What we'd like write_atomic() to derive is the bare
(unqualified) type of *ptr, yet iirc only recent compilers have a way to
obtain that.
Jan
On Tue, 2024-09-10 at 18:05 +0200, Jan Beulich wrote:
> On 10.09.2024 17:28, oleksii.kurochko@gmail.com wrote:
> > On Tue, 2024-09-10 at 11:53 +0200, Jan Beulich wrote:
> > > On 02.09.2024 19:01, Oleksii Kurochko wrote:
> > > > --- a/xen/arch/riscv/include/asm/atomic.h
> > > > +++ b/xen/arch/riscv/include/asm/atomic.h
> > > > @@ -54,16 +54,16 @@ static always_inline void
> > > > read_atomic_size(const volatile void *p,
> > > > })
> > > >
> > > > static always_inline void _write_atomic(volatile void *p,
> > > > - unsigned long x,
> > > > + void *x,
> > >
> > > Pointer-to-const please, to further aid in easily recognizing
> > > which
> > > parameter is what. After all ...
> > >
> > > > unsigned int size)
> > > > {
> > > > switch ( size )
> > > > {
> > > > - case 1: writeb_cpu(x, p); break;
> > > > - case 2: writew_cpu(x, p); break;
> > > > - case 4: writel_cpu(x, p); break;
> > >
> > > ... unhelpfully enough parameters are then swapped, just to
> > > confuse
> > > things.
> > If it would be better to keep 'unsigned long' as the type of x,
> > then,
> > if I am not mistaken, write_atomic() should be updated in the
> > following
> > way:
> > #define write_atomic(p, x) \
> > ({ \
> > typeof(*(p)) x_ = (x); \
> > _write_atomic(p, *(unsigned long *)&x_,
> > sizeof(*(p)));
> > \
> > })
> > However, I am not sure if it is safe when x is a 2-byte value (for
> > example) that it will read more than 2 bytes before passing the
> > value
> > to the _write_atomic() function.
>
> No, that's definitely unsafe.
Then, at the moment, I don't see a better option than having const void
*x as an argument for the _write_atomic() function and then performing
casts when writeX_cpu(*(const uintX *)x, p) is called.
>
> > > > @@ -72,7 +72,7 @@ static always_inline void
> > > > _write_atomic(volatile
> > > > void *p,
> > > > #define write_atomic(p, x) \
> > > > ({ \
> > > > typeof(*(p)) x_ = (x); \
> > > > - _write_atomic(p, x_, sizeof(*(p))); \
> > > > + _write_atomic(p, &x_, sizeof(*(p))); \
> > > > })
> > > >
> > > > static always_inline void _add_sized(volatile void *p,
> > > > @@ -82,27 +82,23 @@ static always_inline void
> > > > _add_sized(volatile
> > > > void *p,
> > > > {
> > > > case 1:
> > > > {
> > > > - volatile uint8_t *ptr = p;
> > > > - write_atomic(ptr, read_atomic(ptr) + x);
> > > > + writeb_cpu(readb_cpu(p) + x, p);
> > > > break;
> > > > }
> > > > case 2:
> > > > {
> > > > - volatile uint16_t *ptr = p;
> > > > - write_atomic(ptr, read_atomic(ptr) + x);
> > > > + writew_cpu(readw_cpu(p) + x, p);
> > > > break;
> > > > }
> > > > case 4:
> > > > {
> > > > - volatile uint32_t *ptr = p;
> > > > - write_atomic(ptr, read_atomic(ptr) + x);
> > > > + writel_cpu(readl_cpu(p) + x, p);
> > > > break;
> > > > }
> > > > #ifndef CONFIG_RISCV_32
> > > > case 8:
> > > > {
> > > > - volatile uint64_t *ptr = p;
> > > > - write_atomic(ptr, read_atomic(ptr) + x);
> > > > + writeq_cpu(readw_cpu(p) + x, p);
> > > > break;
> > > > }
> > > > #endif
> > >
> > > I'm afraid I don't understand this part, or more specifically the
> > > respective
> > > part of the description. It is the first parameter of
> > > write_atomic()
> > > which is
> > > volatile qualified. And it is the first argument that's volatile
> > > qualified
> > > here. Isn't the problem entirely unrelated to volatile-ness, and
> > > instead a
> > > result of the other parameter changing from scalar to pointer
> > > type,
> > > which
> > > doesn't fit the addition expressions you pass in?
> > if _add_sized() is defined as it was before:
> > static always_inline void _add_sized(volatile void *p,
> > unsigned long x, unsigned
> > int
> > size)
> > {
> > switch ( size )
> > {
> > case 1:
> > {
> > volatile uint8_t *ptr = p;
> > write_atomic(ptr, read_atomic(ptr) + x);
> > break;
> > }
> > ...
> > Then write_atomic(ptr, read_atomic(ptr) + x) will be be changed to:
> > volatile uint8_t x_ = (x);
> >
> > And that will cause a compiler error:
> > ./arch/riscv/include/asm/atomic.h:75:22: error: passing argument
> > 2
> > of '_write_atomic' discards 'volatile' qualifier from pointer
> > target
> > type [-Werror=discarded-qualifiers]
> > 75 | _write_atomic(p, &x_, sizeof(*(p)));
> > Because it can't cast 'volatile uint8_t *' to 'void *':
> > expected 'void *' but argument is of type 'volatile uint8_t *'
> > {aka
> > 'volatile unsigned char *'}
>
> Oh, I think I see now. What we'd like write_atomic() to derive is the
> bare
> (unqualified) type of *ptr, yet iirc only recent compilers have a way
> to
> obtain that.
I assume that you are speaking about typeof_unqual which requires C23
(?).
__auto_type seems to me can also drop volatile quilifier but in the
docs I don't see that it should (or not) discard qualifier. Could it be
an option:
#define write_atomic(p, x) \
({ \
__auto_type x_ = (x); \
_write_atomic(p, &x_, sizeof(*(p))); \
})
And another option could be just drop volatile by casting:
#define write_atomic(p, x) \
...
_write_atomic(p, (const void *)&x_, sizeof(*(p)));
~ Oleksii
On 11.09.2024 13:34, oleksii.kurochko@gmail.com wrote:
> On Tue, 2024-09-10 at 18:05 +0200, Jan Beulich wrote:
>> On 10.09.2024 17:28, oleksii.kurochko@gmail.com wrote:
>>> On Tue, 2024-09-10 at 11:53 +0200, Jan Beulich wrote:
>>>> On 02.09.2024 19:01, Oleksii Kurochko wrote:
>>>>> @@ -72,7 +72,7 @@ static always_inline void
>>>>> _write_atomic(volatile
>>>>> void *p,
>>>>> #define write_atomic(p, x) \
>>>>> ({ \
>>>>> typeof(*(p)) x_ = (x); \
>>>>> - _write_atomic(p, x_, sizeof(*(p))); \
>>>>> + _write_atomic(p, &x_, sizeof(*(p))); \
>>>>> })
>>>>>
>>>>> static always_inline void _add_sized(volatile void *p,
>>>>> @@ -82,27 +82,23 @@ static always_inline void
>>>>> _add_sized(volatile
>>>>> void *p,
>>>>> {
>>>>> case 1:
>>>>> {
>>>>> - volatile uint8_t *ptr = p;
>>>>> - write_atomic(ptr, read_atomic(ptr) + x);
>>>>> + writeb_cpu(readb_cpu(p) + x, p);
>>>>> break;
>>>>> }
>>>>> case 2:
>>>>> {
>>>>> - volatile uint16_t *ptr = p;
>>>>> - write_atomic(ptr, read_atomic(ptr) + x);
>>>>> + writew_cpu(readw_cpu(p) + x, p);
>>>>> break;
>>>>> }
>>>>> case 4:
>>>>> {
>>>>> - volatile uint32_t *ptr = p;
>>>>> - write_atomic(ptr, read_atomic(ptr) + x);
>>>>> + writel_cpu(readl_cpu(p) + x, p);
>>>>> break;
>>>>> }
>>>>> #ifndef CONFIG_RISCV_32
>>>>> case 8:
>>>>> {
>>>>> - volatile uint64_t *ptr = p;
>>>>> - write_atomic(ptr, read_atomic(ptr) + x);
>>>>> + writeq_cpu(readw_cpu(p) + x, p);
>>>>> break;
>>>>> }
>>>>> #endif
>>>>
>>>> I'm afraid I don't understand this part, or more specifically the
>>>> respective
>>>> part of the description. It is the first parameter of
>>>> write_atomic()
>>>> which is
>>>> volatile qualified. And it is the first argument that's volatile
>>>> qualified
>>>> here. Isn't the problem entirely unrelated to volatile-ness, and
>>>> instead a
>>>> result of the other parameter changing from scalar to pointer
>>>> type,
>>>> which
>>>> doesn't fit the addition expressions you pass in?
>>> if _add_sized() is defined as it was before:
>>> static always_inline void _add_sized(volatile void *p,
>>> unsigned long x, unsigned
>>> int
>>> size)
>>> {
>>> switch ( size )
>>> {
>>> case 1:
>>> {
>>> volatile uint8_t *ptr = p;
>>> write_atomic(ptr, read_atomic(ptr) + x);
>>> break;
>>> }
>>> ...
>>> Then write_atomic(ptr, read_atomic(ptr) + x) will be be changed to:
>>> volatile uint8_t x_ = (x);
>>>
>>> And that will cause a compiler error:
>>> ./arch/riscv/include/asm/atomic.h:75:22: error: passing argument
>>> 2
>>> of '_write_atomic' discards 'volatile' qualifier from pointer
>>> target
>>> type [-Werror=discarded-qualifiers]
>>> 75 | _write_atomic(p, &x_, sizeof(*(p)));
>>> Because it can't cast 'volatile uint8_t *' to 'void *':
>>> expected 'void *' but argument is of type 'volatile uint8_t *'
>>> {aka
>>> 'volatile unsigned char *'}
>>
>> Oh, I think I see now. What we'd like write_atomic() to derive is the
>> bare
>> (unqualified) type of *ptr, yet iirc only recent compilers have a way
>> to
>> obtain that.
> I assume that you are speaking about typeof_unqual which requires C23
> (?).
What C version it requires doesn't matter much for our purposes. The
question is as of which gcc / clang version (if any) this is supported
as an extension.
> __auto_type seems to me can also drop volatile quilifier but in the
> docs I don't see that it should (or not) discard qualifier. Could it be
> an option:
> #define write_atomic(p, x) \
> ({ \
> __auto_type x_ = (x); \
> _write_atomic(p, &x_, sizeof(*(p))); \
> })
For our purposes __auto_type doesn't provide advantages over typeof().
Plus, more importantly, the use above is wrong, just like typeof(x)
would also be wrong. It needs to be p that the type is derived from.
Otherwise consider what happens when ptr is unsigned long * or
unsigned short * and you write
write_atomic(ptr, 0);
> And another option could be just drop volatile by casting:
> #define write_atomic(p, x) \
> ...
> _write_atomic(p, (const void *)&x_, sizeof(*(p)));
See what I said earlier about casts: You shall not cast away
qualifiers. Besides doing so being bad practice, you'll notice the
latest when RISC-V code also becomes subject to Misra compliance.
Jan
On Wed, 2024-09-11 at 13:49 +0200, Jan Beulich wrote:
> On 11.09.2024 13:34, oleksii.kurochko@gmail.com wrote:
> > On Tue, 2024-09-10 at 18:05 +0200, Jan Beulich wrote:
> > > On 10.09.2024 17:28, oleksii.kurochko@gmail.com wrote:
> > > > On Tue, 2024-09-10 at 11:53 +0200, Jan Beulich wrote:
> > > > > On 02.09.2024 19:01, Oleksii Kurochko wrote:
> > > > > > @@ -72,7 +72,7 @@ static always_inline void
> > > > > > _write_atomic(volatile
> > > > > > void *p,
> > > > > > #define write_atomic(p, x) \
> > > > > > ({ \
> > > > > > typeof(*(p)) x_ = (x); \
> > > > > > - _write_atomic(p, x_, sizeof(*(p))); \
> > > > > > + _write_atomic(p, &x_, sizeof(*(p))); \
> > > > > > })
> > > > > >
> > > > > > static always_inline void _add_sized(volatile void *p,
> > > > > > @@ -82,27 +82,23 @@ static always_inline void
> > > > > > _add_sized(volatile
> > > > > > void *p,
> > > > > > {
> > > > > > case 1:
> > > > > > {
> > > > > > - volatile uint8_t *ptr = p;
> > > > > > - write_atomic(ptr, read_atomic(ptr) + x);
> > > > > > + writeb_cpu(readb_cpu(p) + x, p);
> > > > > > break;
> > > > > > }
> > > > > > case 2:
> > > > > > {
> > > > > > - volatile uint16_t *ptr = p;
> > > > > > - write_atomic(ptr, read_atomic(ptr) + x);
> > > > > > + writew_cpu(readw_cpu(p) + x, p);
> > > > > > break;
> > > > > > }
> > > > > > case 4:
> > > > > > {
> > > > > > - volatile uint32_t *ptr = p;
> > > > > > - write_atomic(ptr, read_atomic(ptr) + x);
> > > > > > + writel_cpu(readl_cpu(p) + x, p);
> > > > > > break;
> > > > > > }
> > > > > > #ifndef CONFIG_RISCV_32
> > > > > > case 8:
> > > > > > {
> > > > > > - volatile uint64_t *ptr = p;
> > > > > > - write_atomic(ptr, read_atomic(ptr) + x);
> > > > > > + writeq_cpu(readw_cpu(p) + x, p);
> > > > > > break;
> > > > > > }
> > > > > > #endif
> > > > >
> > > > > I'm afraid I don't understand this part, or more specifically
> > > > > the
> > > > > respective
> > > > > part of the description. It is the first parameter of
> > > > > write_atomic()
> > > > > which is
> > > > > volatile qualified. And it is the first argument that's
> > > > > volatile
> > > > > qualified
> > > > > here. Isn't the problem entirely unrelated to volatile-ness,
> > > > > and
> > > > > instead a
> > > > > result of the other parameter changing from scalar to pointer
> > > > > type,
> > > > > which
> > > > > doesn't fit the addition expressions you pass in?
> > > > if _add_sized() is defined as it was before:
> > > > static always_inline void _add_sized(volatile void *p,
> > > > unsigned long x,
> > > > unsigned
> > > > int
> > > > size)
> > > > {
> > > > switch ( size )
> > > > {
> > > > case 1:
> > > > {
> > > > volatile uint8_t *ptr = p;
> > > > write_atomic(ptr, read_atomic(ptr) + x);
> > > > break;
> > > > }
> > > > ...
> > > > Then write_atomic(ptr, read_atomic(ptr) + x) will be be changed
> > > > to:
> > > > volatile uint8_t x_ = (x);
> > > >
> > > > And that will cause a compiler error:
> > > > ./arch/riscv/include/asm/atomic.h:75:22: error: passing
> > > > argument
> > > > 2
> > > > of '_write_atomic' discards 'volatile' qualifier from
> > > > pointer
> > > > target
> > > > type [-Werror=discarded-qualifiers]
> > > > 75 | _write_atomic(p, &x_, sizeof(*(p)));
> > > > Because it can't cast 'volatile uint8_t *' to 'void *':
> > > > expected 'void *' but argument is of type 'volatile uint8_t
> > > > *'
> > > > {aka
> > > > 'volatile unsigned char *'}
> > >
> > > Oh, I think I see now. What we'd like write_atomic() to derive is
> > > the
> > > bare
> > > (unqualified) type of *ptr, yet iirc only recent compilers have a
> > > way
> > > to
> > > obtain that.
> > I assume that you are speaking about typeof_unqual which requires
> > C23
> > (?).
>
> What C version it requires doesn't matter much for our purposes. The
> question is as of which gcc / clang version (if any) this is
> supported
> as an extension.
>
> > __auto_type seems to me can also drop volatile quilifier but in the
> > docs I don't see that it should (or not) discard qualifier. Could
> > it be
> > an option:
> > #define write_atomic(p, x) \
> > ({ \
> > __auto_type x_ = (x); \
> > _write_atomic(p, &x_, sizeof(*(p))); \
> > })
>
> For our purposes __auto_type doesn't provide advantages over
> typeof().
> Plus, more importantly, the use above is wrong, just like typeof(x)
> would also be wrong. It needs to be p that the type is derived from.
> Otherwise consider what happens when ptr is unsigned long * or
> unsigned short * and you write
>
> write_atomic(ptr, 0);
>
> > And another option could be just drop volatile by casting:
> > #define write_atomic(p, x) \
> > ...
> > _write_atomic(p, (const void *)&x_,
> > sizeof(*(p)));
>
> See what I said earlier about casts: You shall not cast away
> qualifiers. Besides doing so being bad practice, you'll notice the
> latest when RISC-V code also becomes subject to Misra compliance.
Then probably the best one option will be to use union:
#define write_atomic(p, x)
\
({
\
union { typeof(*(p)) val; char c[sizeof(*(p))]; } x_ = { .val =
(x) }; \
_write_atomic(p, x_.c, sizeof(*(p)));
\
})
~ Oleksii
On Thu, 2024-09-12 at 13:15 +0200, oleksii.kurochko@gmail.com wrote:
> On Wed, 2024-09-11 at 13:49 +0200, Jan Beulich wrote:
> > On 11.09.2024 13:34, oleksii.kurochko@gmail.com wrote:
> > > On Tue, 2024-09-10 at 18:05 +0200, Jan Beulich wrote:
> > > > On 10.09.2024 17:28, oleksii.kurochko@gmail.com wrote:
> > > > > On Tue, 2024-09-10 at 11:53 +0200, Jan Beulich wrote:
> > > > > > On 02.09.2024 19:01, Oleksii Kurochko wrote:
> > > > > > > @@ -72,7 +72,7 @@ static always_inline void
> > > > > > > _write_atomic(volatile
> > > > > > > void *p,
> > > > > > > #define write_atomic(p, x)
> > > > > > > \
> > > > > > > ({
> > > > > > > \
> > > > > > > typeof(*(p)) x_ = (x);
> > > > > > > \
> > > > > > > - _write_atomic(p, x_, sizeof(*(p)));
> > > > > > > \
> > > > > > > + _write_atomic(p, &x_, sizeof(*(p)));
> > > > > > > \
> > > > > > > })
> > > > > > >
> > > > > > > static always_inline void _add_sized(volatile void *p,
> > > > > > > @@ -82,27 +82,23 @@ static always_inline void
> > > > > > > _add_sized(volatile
> > > > > > > void *p,
> > > > > > > {
> > > > > > > case 1:
> > > > > > > {
> > > > > > > - volatile uint8_t *ptr = p;
> > > > > > > - write_atomic(ptr, read_atomic(ptr) + x);
> > > > > > > + writeb_cpu(readb_cpu(p) + x, p);
> > > > > > > break;
> > > > > > > }
> > > > > > > case 2:
> > > > > > > {
> > > > > > > - volatile uint16_t *ptr = p;
> > > > > > > - write_atomic(ptr, read_atomic(ptr) + x);
> > > > > > > + writew_cpu(readw_cpu(p) + x, p);
> > > > > > > break;
> > > > > > > }
> > > > > > > case 4:
> > > > > > > {
> > > > > > > - volatile uint32_t *ptr = p;
> > > > > > > - write_atomic(ptr, read_atomic(ptr) + x);
> > > > > > > + writel_cpu(readl_cpu(p) + x, p);
> > > > > > > break;
> > > > > > > }
> > > > > > > #ifndef CONFIG_RISCV_32
> > > > > > > case 8:
> > > > > > > {
> > > > > > > - volatile uint64_t *ptr = p;
> > > > > > > - write_atomic(ptr, read_atomic(ptr) + x);
> > > > > > > + writeq_cpu(readw_cpu(p) + x, p);
> > > > > > > break;
> > > > > > > }
> > > > > > > #endif
> > > > > >
> > > > > > I'm afraid I don't understand this part, or more
> > > > > > specifically
> > > > > > the
> > > > > > respective
> > > > > > part of the description. It is the first parameter of
> > > > > > write_atomic()
> > > > > > which is
> > > > > > volatile qualified. And it is the first argument that's
> > > > > > volatile
> > > > > > qualified
> > > > > > here. Isn't the problem entirely unrelated to volatile-
> > > > > > ness,
> > > > > > and
> > > > > > instead a
> > > > > > result of the other parameter changing from scalar to
> > > > > > pointer
> > > > > > type,
> > > > > > which
> > > > > > doesn't fit the addition expressions you pass in?
> > > > > if _add_sized() is defined as it was before:
> > > > > static always_inline void _add_sized(volatile void *p,
> > > > > unsigned long x,
> > > > > unsigned
> > > > > int
> > > > > size)
> > > > > {
> > > > > switch ( size )
> > > > > {
> > > > > case 1:
> > > > > {
> > > > > volatile uint8_t *ptr = p;
> > > > > write_atomic(ptr, read_atomic(ptr) + x);
> > > > > break;
> > > > > }
> > > > > ...
> > > > > Then write_atomic(ptr, read_atomic(ptr) + x) will be be
> > > > > changed
> > > > > to:
> > > > > volatile uint8_t x_ = (x);
> > > > >
> > > > > And that will cause a compiler error:
> > > > > ./arch/riscv/include/asm/atomic.h:75:22: error: passing
> > > > > argument
> > > > > 2
> > > > > of '_write_atomic' discards 'volatile' qualifier from
> > > > > pointer
> > > > > target
> > > > > type [-Werror=discarded-qualifiers]
> > > > > 75 | _write_atomic(p, &x_, sizeof(*(p)));
> > > > > Because it can't cast 'volatile uint8_t *' to 'void *':
> > > > > expected 'void *' but argument is of type 'volatile
> > > > > uint8_t
> > > > > *'
> > > > > {aka
> > > > > 'volatile unsigned char *'}
> > > >
> > > > Oh, I think I see now. What we'd like write_atomic() to derive
> > > > is
> > > > the
> > > > bare
> > > > (unqualified) type of *ptr, yet iirc only recent compilers have
> > > > a
> > > > way
> > > > to
> > > > obtain that.
> > > I assume that you are speaking about typeof_unqual which requires
> > > C23
> > > (?).
> >
> > What C version it requires doesn't matter much for our purposes.
> > The
> > question is as of which gcc / clang version (if any) this is
> > supported
> > as an extension.
> >
> > > __auto_type seems to me can also drop volatile quilifier but in
> > > the
> > > docs I don't see that it should (or not) discard qualifier. Could
> > > it be
> > > an option:
> > > #define write_atomic(p, x) \
> > > ({ \
> > > __auto_type x_ = (x); \
> > > _write_atomic(p, &x_, sizeof(*(p))); \
> > > })
> >
> > For our purposes __auto_type doesn't provide advantages over
> > typeof().
> > Plus, more importantly, the use above is wrong, just like typeof(x)
> > would also be wrong. It needs to be p that the type is derived
> > from.
> > Otherwise consider what happens when ptr is unsigned long * or
> > unsigned short * and you write
> >
> > write_atomic(ptr, 0);
> >
> > > And another option could be just drop volatile by casting:
> > > #define write_atomic(p, x) \
> > > ...
> > > _write_atomic(p, (const void *)&x_,
> > > sizeof(*(p)));
> >
> > See what I said earlier about casts: You shall not cast away
> > qualifiers. Besides doing so being bad practice, you'll notice the
> > latest when RISC-V code also becomes subject to Misra compliance.
>
> Then probably the best one option will be to use union:
> #define write_atomic(p, x)
> \
> ({
> \
> union { typeof(*(p)) val; char c[sizeof(*(p))]; } x_ = { .val
> =
> (x) }; \
> _write_atomic(p, x_.c, sizeof(*(p)));
> \
> })
Or maybe we can use 'unsigned long' instead of char c[] and then the
casts inside _write_atomic() could be dropped as we can start to use
_write_atomic(..., const unsigned long x, ...).
But then probably it will be good to init: x_.c = 0UL to be sure that
when type of val is uint8_t for example then the significant bytes of
'union {...; unsigned long c}' are 0.
~ Oleksii
© 2016 - 2026 Red Hat, Inc.