| Message ID | 20240205-counter-align-fix-v1-1-4821ced960ab@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | counter: fix privdata alignment | expand |
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
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á
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
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á
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
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)