[PATCH v1] asm-generic: introduce be56 unaligned accessors

[PATCH v1] asm-generic: introduce be56 unaligned accessors

[marius.cristea]

From: Marius Cristea <marius.cristea@microchip.com>

The PAC194X, IIO driver, is using some unaligned 56 bit registers.
Provide some helper accessors in preparation for the new driver.

Signed-off-by: Marius Cristea <marius.cristea@microchip.com>
---
 include/asm-generic/unaligned.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/include/asm-generic/unaligned.h b/include/asm-generic/unaligned.h
index a84c64e5f11e..d171a9f2377a 100644
--- a/include/asm-generic/unaligned.h
+++ b/include/asm-generic/unaligned.h
@@ -152,4 +152,31 @@ static inline u64 get_unaligned_be48(const void *p)
 	return __get_unaligned_be48(p);
 }
 
+static inline void __put_unaligned_be56(const u64 val, u8 *p)
+{
+	*p++ = (val >> 48) & 0xff;
+	*p++ = (val >> 40) & 0xff;
+	*p++ = (val >> 32) & 0xff;
+	*p++ = (val >> 24) & 0xff;
+	*p++ = (val >> 16) & 0xff;
+	*p++ = (val >> 8) & 0xff;
+	*p++ = val & 0xff;
+}
+
+static inline void put_unaligned_be56(const u64 val, void *p)
+{
+	__put_unaligned_be56(val, p);
+}
+
+static inline u64 __get_unaligned_be56(const u8 *p)
+{
+	return (u64)p[0] << 48 | (u64)p[1] << 40 | (u64)p[2] << 32 |
+		(u64)p[3] << 24 | p[4] << 16 | p[5] << 8 | p[6];
+}
+
+static inline u64 get_unaligned_be56(const void *p)
+{
+	return __get_unaligned_be56(p);
+}
+
 #endif /* __ASM_GENERIC_UNALIGNED_H */

base-commit: b82c1d235a30622177ce10dcb94dfd691a49922f
-- 
2.43.0

RE: [PATCH v1] asm-generic: introduce be56 unaligned accessors

[David Laight]

From: marius.cristea@microchip.com
> Sent: 27 September 2024 09:36
> 
> The PAC194X, IIO driver, is using some unaligned 56 bit registers.
> Provide some helper accessors in preparation for the new driver.

Someone please shoot the hardware engineer ;-)

Do separate unaligned access of the first 4 bytes and last four bytes.
It can't matter if the middle byte is accessed twice.

Or, for reads read 8 bytes from 'p & ~1ul' and then fixup
the value.

	David

> 
> Signed-off-by: Marius Cristea <marius.cristea@microchip.com>
> ---
>  include/asm-generic/unaligned.h | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/include/asm-generic/unaligned.h b/include/asm-generic/unaligned.h
> index a84c64e5f11e..d171a9f2377a 100644
> --- a/include/asm-generic/unaligned.h
> +++ b/include/asm-generic/unaligned.h
> @@ -152,4 +152,31 @@ static inline u64 get_unaligned_be48(const void *p)
>  	return __get_unaligned_be48(p);
>  }
> 
> +static inline void __put_unaligned_be56(const u64 val, u8 *p)
> +{
> +	*p++ = (val >> 48) & 0xff;
> +	*p++ = (val >> 40) & 0xff;
> +	*p++ = (val >> 32) & 0xff;
> +	*p++ = (val >> 24) & 0xff;
> +	*p++ = (val >> 16) & 0xff;
> +	*p++ = (val >> 8) & 0xff;
> +	*p++ = val & 0xff;
> +}
> +
> +static inline void put_unaligned_be56(const u64 val, void *p)
> +{
> +	__put_unaligned_be56(val, p);
> +}
> +
> +static inline u64 __get_unaligned_be56(const u8 *p)
> +{
> +	return (u64)p[0] << 48 | (u64)p[1] << 40 | (u64)p[2] << 32 |
> +		(u64)p[3] << 24 | p[4] << 16 | p[5] << 8 | p[6];
> +}
> +
> +static inline u64 get_unaligned_be56(const void *p)
> +{
> +	return __get_unaligned_be56(p);
> +}
> +
>  #endif /* __ASM_GENERIC_UNALIGNED_H */
> 
> base-commit: b82c1d235a30622177ce10dcb94dfd691a49922f
> --
> 2.43.0
> 

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v1] asm-generic: introduce be56 unaligned accessors

[Marius.Cristea]

Hi David,

  Thank you very much for the feedback.

On Sun, 2024-09-29 at 21:16 +0000, David Laight wrote:
> [You don't often get email from david.laight@aculab.com. Learn why
> this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> From: marius.cristea@microchip.com
> > Sent: 27 September 2024 09:36
> > 
> > The PAC194X, IIO driver, is using some unaligned 56 bit registers.
> > Provide some helper accessors in preparation for the new driver.
> 
> Someone please shoot the hardware engineer ;-)
> 
> Do separate unaligned access of the first 4 bytes and last four
> bytes.
> It can't matter if the middle byte is accessed twice.
> 
> Or, for reads read 8 bytes from 'p & ~1ul' and then fixup
> the value.
> 

Do you recommend me to drop this patch?

I know that there are some "workarounds", but those didn't "looks"
nice. I was using that function locally and I got a suggestion from the
IIO subsystem maintainer to move it into the kernel in order for others
to used it.


Thanks,
Marius



>         David
> 
> > 
> > Signed-off-by: Marius Cristea <marius.cristea@microchip.com>
> > ---
> >  include/asm-generic/unaligned.h | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/include/asm-generic/unaligned.h b/include/asm-
> > generic/unaligned.h
> > index a84c64e5f11e..d171a9f2377a 100644
> > --- a/include/asm-generic/unaligned.h
> > +++ b/include/asm-generic/unaligned.h
> > @@ -152,4 +152,31 @@ static inline u64 get_unaligned_be48(const
> > void *p)
> >       return __get_unaligned_be48(p);
> >  }
> > 
> > +static inline void __put_unaligned_be56(const u64 val, u8 *p)
> > +{
> > +     *p++ = (val >> 48) & 0xff;
> > +     *p++ = (val >> 40) & 0xff;
> > +     *p++ = (val >> 32) & 0xff;
> > +     *p++ = (val >> 24) & 0xff;
> > +     *p++ = (val >> 16) & 0xff;
> > +     *p++ = (val >> 8) & 0xff;
> > +     *p++ = val & 0xff;
> > +}
> > +
> > +static inline void put_unaligned_be56(const u64 val, void *p)
> > +{
> > +     __put_unaligned_be56(val, p);
> > +}
> > +
> > +static inline u64 __get_unaligned_be56(const u8 *p)
> > +{
> > +     return (u64)p[0] << 48 | (u64)p[1] << 40 | (u64)p[2] << 32 |
> > +             (u64)p[3] << 24 | p[4] << 16 | p[5] << 8 | p[6];
> > +}
> > +
> > +static inline u64 get_unaligned_be56(const void *p)
> > +{
> > +     return __get_unaligned_be56(p);
> > +}
> > +
> >  #endif /* __ASM_GENERIC_UNALIGNED_H */
> > 
> > base-commit: b82c1d235a30622177ce10dcb94dfd691a49922f
> > --
> > 2.43.0
> > 
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

Re: [PATCH v1] asm-generic: introduce be56 unaligned accessors

[Arnd Bergmann]

On Mon, Oct 7, 2024, at 14:40, Marius.Cristea@microchip.com wrote:
> On Sun, 2024-09-29 at 21:16 +0000, David Laight wrote:
>> [You don't often get email from david.laight@aculab.com. Learn why
>> this is important at https://aka.ms/LearnAboutSenderIdentification ]
>> 
>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>> know the content is safe
>> 
>> From: marius.cristea@microchip.com
>> > Sent: 27 September 2024 09:36
>> > 
>> > The PAC194X, IIO driver, is using some unaligned 56 bit registers.
>> > Provide some helper accessors in preparation for the new driver.
>> 
>> Someone please shoot the hardware engineer ;-)
>> 
>> Do separate unaligned access of the first 4 bytes and last four
>> bytes.
>> It can't matter if the middle byte is accessed twice.
>> 
>> Or, for reads read 8 bytes from 'p & ~1ul' and then fixup
>> the value.
>> 
>
> Do you recommend me to drop this patch?
>
> I know that there are some "workarounds", but those didn't "looks"
> nice. I was using that function locally and I got a suggestion from the
> IIO subsystem maintainer to move it into the kernel in order for others
> to used it.

My feeling is that this is too specific to one driver, I don't
expect it to be shared much. I also suspect that most 56-bit
integers in data structures are actually always part of a naturally
aligned 64-bit word. If that is the case here, the driver can
probably better access it as a normal 64-bit number and mask
out the upper 56 bits using the include/linux/bitfield.h helpers.

        Arnd

Re: [PATCH v1] asm-generic: introduce be56 unaligned accessors

[Marius.Cristea]

On Mon, 2024-10-07 at 14:44 +0000, Arnd Bergmann wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Mon, Oct 7, 2024, at 14:40, Marius.Cristea@microchip.com wrote:
> > On Sun, 2024-09-29 at 21:16 +0000, David Laight wrote:
> > > [You don't often get email from david.laight@aculab.com. Learn
> > > why
> > > this is important at
> > > https://aka.ms/LearnAboutSenderIdentification ]
> > > 
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > > 
> > > From: marius.cristea@microchip.com
> > > > Sent: 27 September 2024 09:36
> > > > 
> > > > The PAC194X, IIO driver, is using some unaligned 56 bit
> > > > registers.
> > > > Provide some helper accessors in preparation for the new
> > > > driver.
> > > 
> > > Someone please shoot the hardware engineer ;-)
> > > 
> > > Do separate unaligned access of the first 4 bytes and last four
> > > bytes.
> > > It can't matter if the middle byte is accessed twice.
> > > 
> > > Or, for reads read 8 bytes from 'p & ~1ul' and then fixup
> > > the value.
> > > 
> > 
> > Do you recommend me to drop this patch?
> > 
> > I know that there are some "workarounds", but those didn't "looks"
> > nice. I was using that function locally and I got a suggestion from
> > the
> > IIO subsystem maintainer to move it into the kernel in order for
> > others
> > to used it.
> 
> My feeling is that this is too specific to one driver, I don't
> expect it to be shared much. I also suspect that most 56-bit
> integers in data structures are actually always part of a naturally
> aligned 64-bit word. If that is the case here, the driver can
> probably better access it as a normal 64-bit number and mask
> out the upper 56 bits using the include/linux/bitfield.h helpers.
> 
>         Arnd

Most probably this request is quite specific to my driver and I'm not
sure how often it will be used by somebody else.

I'm using block read in order to get multiple registers at a time
(around 76 bytes) and to increase the efficiency of the transfer over
I2C. Being a block read there are different registers length involved
from 16 up to 56 bits long and I need to unpack.

//Marius

Re: [PATCH v1] asm-generic: introduce be56 unaligned accessors

[Arnd Bergmann]

On Mon, Oct 7, 2024, at 14:57, Marius.Cristea@microchip.com wrote:
> On Mon, 2024-10-07 at 14:44 +0000, Arnd Bergmann wrote:
>
> Most probably this request is quite specific to my driver and I'm not
> sure how often it will be used by somebody else.
>
> I'm using block read in order to get multiple registers at a time
> (around 76 bytes) and to increase the efficiency of the transfer over
> I2C. Being a block read there are different registers length involved
> from 16 up to 56 bits long and I need to unpack.

Ok, makes sense. In this case I would keep the exact implementation
you have but move it into your driver where I guess it started out.
If we ever get multiple drivers that need the same thing, we can
still consolidate the implementation.

       Arnd

RE: [PATCH v1] asm-generic: introduce be56 unaligned accessors

[David Laight]

...
> I'm using block read in order to get multiple registers at a time
> (around 76 bytes) and to increase the efficiency of the transfer over
> I2C. Being a block read there are different registers length involved
> from 16 up to 56 bits long and I need to unpack.

You could do an unaligned 64bit BE read and then shift the value right 8 bits
(and only advance the pointer 7 bytes).
Safe because you can guarantee a spare byte at the end of the data.

On x86-64 you could do that for all sizes!

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH v1] asm-generic: introduce be56 unaligned accessors

[David Laight]

From: Arnd Bergmann
> Sent: 07 October 2024 15:45
> On Mon, Oct 7, 2024, at 14:40, Marius.Cristea@microchip.com wrote:
> > On Sun, 2024-09-29 at 21:16 +0000, David Laight wrote:
> >> [You don't often get email from david.laight@aculab.com. Learn why
> >> this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >>
> >> EXTERNAL EMAIL: Do not click links or open attachments unless you
> >> know the content is safe
> >>
> >> From: marius.cristea@microchip.com
> >> > Sent: 27 September 2024 09:36
> >> >
> >> > The PAC194X, IIO driver, is using some unaligned 56 bit registers.
> >> > Provide some helper accessors in preparation for the new driver.
> >>
> >> Someone please shoot the hardware engineer ;-)
> >>
> >> Do separate unaligned access of the first 4 bytes and last four
> >> bytes.
> >> It can't matter if the middle byte is accessed twice.
> >>
> >> Or, for reads read 8 bytes from 'p & ~1ul' and then fixup
> >> the value.
> >>
> >
> > Do you recommend me to drop this patch?
> >
> > I know that there are some "workarounds", but those didn't "looks"
> > nice. I was using that function locally and I got a suggestion from the
> > IIO subsystem maintainer to move it into the kernel in order for others
> > to used it.
> 
> My feeling is that this is too specific to one driver, I don't
> expect it to be shared much. I also suspect that most 56-bit
> integers in data structures are actually always part of a naturally
> aligned 64-bit word. If that is the case here, the driver can
> probably better access it as a normal 64-bit number and mask
> out the upper 56 bits using the include/linux/bitfield.h helpers.

Or get the compiler to do it for you.
This DTRT https://www.godbolt.org/z/GMdePjYMY:

struct foo {
    unsigned long a;
    unsigned char b;
    unsigned long c:56 __attribute__((packed));
    unsigned long d;
};

Although you'll need #define htobe56(x) (htobe64(x) >> 8) on LE.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)