[PATCH] counter: fix privdata alignment

[PATCH] counter: fix privdata alignment

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

Aligning to the L1 cache does guarantee the same alignment as kmallocing
an object [1]. Furthermore, in some platforms, that alignment is not
sufficient for DMA safety (in case someone wants to have a DMA safe
buffer in privdata) [2].

Sometime ago, we had the same fixes in IIO.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/devres.c#n35
[2]: https://lore.kernel.org/linux-iio/20220508175712.647246-2-jic23@kernel.org/

Fixes: c18e2760308e ("counter: Provide alternative counter registration functions")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
William, if you prefer, we can do something like in IIO and add a
specific COUNTER_DMA_MINALIGN define
---
 drivers/counter/counter-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
index 09c77afb33ca..073bf6b67a57 100644
--- a/drivers/counter/counter-core.c
+++ b/drivers/counter/counter-core.c
@@ -34,7 +34,7 @@ struct counter_device_allochelper {
 	 * This is cache line aligned to ensure private data behaves like if it
 	 * were kmalloced separately.
 	 */
-	unsigned long privdata[] ____cacheline_aligned;
+	unsigned long privdata[] __aligned(ARCH_DMA_MINALIGN);
 };
 
 static void counter_device_release(struct device *dev)

---
base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
change-id: 20240205-counter-align-fix-3faebfb572af
--

Thanks!
- Nuno Sá

Re: [PATCH] counter: fix privdata alignment

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 1849 bytes --]

On Mon, Feb 05, 2024 at 04:58:14PM +0100, Nuno Sa via B4 Relay wrote:
> From: Nuno Sa <nuno.sa@analog.com>
> 
> Aligning to the L1 cache does guarantee the same alignment as kmallocing
> an object [1]. Furthermore, in some platforms, that alignment is not
> sufficient for DMA safety (in case someone wants to have a DMA safe
> buffer in privdata) [2].
> 
> Sometime ago, we had the same fixes in IIO.
> 
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/devres.c#n35
> [2]: https://lore.kernel.org/linux-iio/20220508175712.647246-2-jic23@kernel.org/
> 
> Fixes: c18e2760308e ("counter: Provide alternative counter registration functions")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
> William, if you prefer, we can do something like in IIO and add a
> specific COUNTER_DMA_MINALIGN define
> ---
>  drivers/counter/counter-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
> index 09c77afb33ca..073bf6b67a57 100644
> --- a/drivers/counter/counter-core.c
> +++ b/drivers/counter/counter-core.c
> @@ -34,7 +34,7 @@ struct counter_device_allochelper {
>  	 * This is cache line aligned to ensure private data behaves like if it
>  	 * were kmalloced separately.
>  	 */
> -	unsigned long privdata[] ____cacheline_aligned;
> +	unsigned long privdata[] __aligned(ARCH_DMA_MINALIGN);
>  };
>  
>  static void counter_device_release(struct device *dev)
> 
> ---
> base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
> change-id: 20240205-counter-align-fix-3faebfb572af
> --
> 
> Thanks!
> - Nuno Sá

Hi Nunon,

This change sounds reasonable, but should the comment block above
privdata be updated to reflect the change?

Sincerely,

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] counter: fix privdata alignment

[Nuno Sá]

On Thu, 2024-02-08 at 13:34 -0500, William Breathitt Gray wrote:
> On Mon, Feb 05, 2024 at 04:58:14PM +0100, Nuno Sa via B4 Relay wrote:
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > Aligning to the L1 cache does guarantee the same alignment as kmallocing
> > an object [1]. Furthermore, in some platforms, that alignment is not
> > sufficient for DMA safety (in case someone wants to have a DMA safe
> > buffer in privdata) [2].
> > 
> > Sometime ago, we had the same fixes in IIO.
> > 
> > [1]:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/devres.c#n35
> > [2]:
> > https://lore.kernel.org/linux-iio/20220508175712.647246-2-jic23@kernel.org/
> > 
> > Fixes: c18e2760308e ("counter: Provide alternative counter registration
> > functions")
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> > William, if you prefer, we can do something like in IIO and add a
> > specific COUNTER_DMA_MINALIGN define
> > ---
> >  drivers/counter/counter-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
> > index 09c77afb33ca..073bf6b67a57 100644
> > --- a/drivers/counter/counter-core.c
> > +++ b/drivers/counter/counter-core.c
> > @@ -34,7 +34,7 @@ struct counter_device_allochelper {
> >  	 * This is cache line aligned to ensure private data behaves like
> > if it
> >  	 * were kmalloced separately.
> >  	 */
> > -	unsigned long privdata[] ____cacheline_aligned;
> > +	unsigned long privdata[] __aligned(ARCH_DMA_MINALIGN);
> >  };
> >  
> >  static void counter_device_release(struct device *dev)
> > 
> > ---
> > base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
> > change-id: 20240205-counter-align-fix-3faebfb572af
> > --
> > 
> > Thanks!
> > - Nuno Sá
> 
> Hi Nunon,
> 
> This change sounds reasonable, but should the comment block above
> privdata be updated to reflect the change?
> 
> 
Hi William,

Yeah, maybe. I can spin a new version with that... To be sure, you mean (in the
comment) private -> privdata, right?

Also realized a typo in the commit message:

"Aligning to the L1 cache does guarantee..." - Obviously, I meant "does
*not*...".

Thanks!
- Nuno Sá

Re: [PATCH] counter: fix privdata alignment

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1479 bytes --]

Hi Nuno,

On Fri, Feb 09, 2024 at 08:42:02AM +0100, Nuno Sá wrote:
> On Thu, 2024-02-08 at 13:34 -0500, William Breathitt Gray wrote:
> > On Mon, Feb 05, 2024 at 04:58:14PM +0100, Nuno Sa via B4 Relay wrote:
> > > diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
> > > index 09c77afb33ca..073bf6b67a57 100644
> > > --- a/drivers/counter/counter-core.c
> > > +++ b/drivers/counter/counter-core.c
> > > @@ -34,7 +34,7 @@ struct counter_device_allochelper {
> > >  	 * This is cache line aligned to ensure private data behaves like if it
> > >  	 * were kmalloced separately.
> > >  	 */
> > > -	unsigned long privdata[] ____cacheline_aligned;
> > > +	unsigned long privdata[] __aligned(ARCH_DMA_MINALIGN);
> > >  };
> > >  
> > >  static void counter_device_release(struct device *dev)
> > > 
> > 
> > This change sounds reasonable, but should the comment block above
> > privdata be updated to reflect the change?
> 
> Yeah, maybe. I can spin a new version with that... To be sure, you mean (in the
> comment) private -> privdata, right?

I guess he means: "This is cache line aligned to ensure private data
behaves like if it were kmalloced separately." After your change it's
not cache line aligned any more. IMHO keeping "private" is fine.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] counter: fix privdata alignment

[Nuno Sá]

On Fri, 2024-02-09 at 09:30 +0100, Uwe Kleine-König wrote:
> Hi Nuno,
> 
> On Fri, Feb 09, 2024 at 08:42:02AM +0100, Nuno Sá wrote:
> > On Thu, 2024-02-08 at 13:34 -0500, William Breathitt Gray wrote:
> > > On Mon, Feb 05, 2024 at 04:58:14PM +0100, Nuno Sa via B4 Relay wrote:
> > > > diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-
> > > > core.c
> > > > index 09c77afb33ca..073bf6b67a57 100644
> > > > --- a/drivers/counter/counter-core.c
> > > > +++ b/drivers/counter/counter-core.c
> > > > @@ -34,7 +34,7 @@ struct counter_device_allochelper {
> > > >  	 * This is cache line aligned to ensure private data behaves
> > > > like if it
> > > >  	 * were kmalloced separately.
> > > >  	 */
> > > > -	unsigned long privdata[] ____cacheline_aligned;
> > > > +	unsigned long privdata[] __aligned(ARCH_DMA_MINALIGN);
> > > >  };
> > > >  
> > > >  static void counter_device_release(struct device *dev)
> > > > 
> > > 
> > > This change sounds reasonable, but should the comment block above
> > > privdata be updated to reflect the change?
> > 
> > Yeah, maybe. I can spin a new version with that... To be sure, you mean (in
> > the
> > comment) private -> privdata, right?
> 
> I guess he means: "This is cache line aligned to ensure private data
> behaves like if it were kmalloced separately." After your change it's
> not cache line aligned any more. IMHO keeping "private" is fine.
> 
> 

Oh yeah...

Yeah, it will depend on the platform. In some, it will still be cache aligned
but in others (as x86 which is DMA coeherent I think), it won't be and we can
actually safe some memory.

- Nuno Sá

Re: [PATCH] counter: fix privdata alignment

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 2109 bytes --]

On Fri, Feb 09, 2024 at 10:07:19AM +0100, Nuno Sá wrote:
> On Fri, 2024-02-09 at 09:30 +0100, Uwe Kleine-König wrote:
> > Hi Nuno,
> > 
> > On Fri, Feb 09, 2024 at 08:42:02AM +0100, Nuno Sá wrote:
> > > On Thu, 2024-02-08 at 13:34 -0500, William Breathitt Gray wrote:
> > > > On Mon, Feb 05, 2024 at 04:58:14PM +0100, Nuno Sa via B4 Relay wrote:
> > > > > diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-
> > > > > core.c
> > > > > index 09c77afb33ca..073bf6b67a57 100644
> > > > > --- a/drivers/counter/counter-core.c
> > > > > +++ b/drivers/counter/counter-core.c
> > > > > @@ -34,7 +34,7 @@ struct counter_device_allochelper {
> > > > >  	 * This is cache line aligned to ensure private data behaves
> > > > > like if it
> > > > >  	 * were kmalloced separately.
> > > > >  	 */
> > > > > -	unsigned long privdata[] ____cacheline_aligned;
> > > > > +	unsigned long privdata[] __aligned(ARCH_DMA_MINALIGN);
> > > > >  };
> > > > >  
> > > > >  static void counter_device_release(struct device *dev)
> > > > > 
> > > > 
> > > > This change sounds reasonable, but should the comment block above
> > > > privdata be updated to reflect the change?
> > > 
> > > Yeah, maybe. I can spin a new version with that... To be sure, you mean (in
> > > the
> > > comment) private -> privdata, right?
> > 
> > I guess he means: "This is cache line aligned to ensure private data
> > behaves like if it were kmalloced separately." After your change it's
> > not cache line aligned any more. IMHO keeping "private" is fine.
> > 
> > 
> 
> Oh yeah...
> 
> Yeah, it will depend on the platform. In some, it will still be cache aligned
> but in others (as x86 which is DMA coeherent I think), it won't be and we can
> actually safe some memory.
> 
> - Nuno Sá

Yes, I was referring to the possibility that it won't be cache aligned
anymore in some platforms as you mentioned, so a different comment would
be better there now. You can keep the "private" wording if you like, or
use "privdata" if you think it's clearer.

Thanks,

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]