2022-09-09 12:32:44

by Nam Cao

[permalink] [raw]
Subject: [PATCH v3 1/2] staging: vt6655: remove unnecessary volatile qualifier

Remove volatile qualifier for the member rd0 of struct vnt_rx_desc,
because there is no reason it must be volatile.

Signed-off-by: Nam Cao <[email protected]>
---
drivers/staging/vt6655/desc.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vt6655/desc.h b/drivers/staging/vt6655/desc.h
index 17a40c53b8ff..3f0f287b1693 100644
--- a/drivers/staging/vt6655/desc.h
+++ b/drivers/staging/vt6655/desc.h
@@ -182,7 +182,7 @@ struct vnt_rdes1 {

/* Rx descriptor*/
struct vnt_rx_desc {
- volatile struct vnt_rdes0 rd0;
+ struct vnt_rdes0 rd0;
volatile struct vnt_rdes1 rd1;
volatile __le32 buff_addr;
volatile __le32 next_desc;
--
2.25.1


2022-09-09 18:06:20

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH v3 1/2] staging: vt6655: remove unnecessary volatile qualifier

On Fri, Sep 09, 2022 at 02:17:55PM +0200, Nam Cao wrote:
> Remove volatile qualifier for the member rd0 of struct vnt_rx_desc,
> because there is no reason it must be volatile.
>
> Signed-off-by: Nam Cao <[email protected]>
> ---
> drivers/staging/vt6655/desc.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/vt6655/desc.h b/drivers/staging/vt6655/desc.h
> index 17a40c53b8ff..3f0f287b1693 100644
> --- a/drivers/staging/vt6655/desc.h
> +++ b/drivers/staging/vt6655/desc.h
> @@ -182,7 +182,7 @@ struct vnt_rdes1 {
>
> /* Rx descriptor*/
> struct vnt_rx_desc {
> - volatile struct vnt_rdes0 rd0;
> + struct vnt_rdes0 rd0;

You can not just remove this without describing _WHY_ it is ok to do so.

Have you properly determined why it is, or is not, ok to use volatile
here? And if so, why are you leaving:

> volatile struct vnt_rdes1 rd1;
> volatile __le32 buff_addr;
> volatile __le32 next_desc;

Those?

Please read up on why volatile almost never makes any sense in the
kernel (note, sometimes it does), and please write a better changelog
text for when you submit the next version of this patch series.

thanks,

greg k-h

2022-09-11 07:17:19

by Nam Cao

[permalink] [raw]
Subject: Re: [PATCH v3 1/2] staging: vt6655: remove unnecessary volatile qualifier

On Fri, Sep 9, 2022 at 8:03 PM Greg KH <[email protected]> wrote:
>
> On Fri, Sep 09, 2022 at 02:17:55PM +0200, Nam Cao wrote:
> > Remove volatile qualifier for the member rd0 of struct vnt_rx_desc,
> > because there is no reason it must be volatile.
> >
> > Signed-off-by: Nam Cao <[email protected]>
> > ---
> > drivers/staging/vt6655/desc.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/vt6655/desc.h b/drivers/staging/vt6655/desc.h
> > index 17a40c53b8ff..3f0f287b1693 100644
> > --- a/drivers/staging/vt6655/desc.h
> > +++ b/drivers/staging/vt6655/desc.h
> > @@ -182,7 +182,7 @@ struct vnt_rdes1 {
> >
> > /* Rx descriptor*/
> > struct vnt_rx_desc {
> > - volatile struct vnt_rdes0 rd0;
> > + struct vnt_rdes0 rd0;
>
> You can not just remove this without describing _WHY_ it is ok to do so.
>
> Have you properly determined why it is, or is not, ok to use volatile
> here?

I did not carefully look at the volatile usage here. After looking at it
again, using volatile is actually valid: the structure resides on coherent
memory.

Sorry for being careless.

Best regards,
Nam

2022-09-11 07:54:17

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH v3 1/2] staging: vt6655: remove unnecessary volatile qualifier

On Sun, Sep 11, 2022 at 09:12:44AM +0200, Nam Cao wrote:
> On Fri, Sep 9, 2022 at 8:03 PM Greg KH <[email protected]> wrote:
> >
> > On Fri, Sep 09, 2022 at 02:17:55PM +0200, Nam Cao wrote:
> > > Remove volatile qualifier for the member rd0 of struct vnt_rx_desc,
> > > because there is no reason it must be volatile.
> > >
> > > Signed-off-by: Nam Cao <[email protected]>
> > > ---
> > > drivers/staging/vt6655/desc.h | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/vt6655/desc.h b/drivers/staging/vt6655/desc.h
> > > index 17a40c53b8ff..3f0f287b1693 100644
> > > --- a/drivers/staging/vt6655/desc.h
> > > +++ b/drivers/staging/vt6655/desc.h
> > > @@ -182,7 +182,7 @@ struct vnt_rdes1 {
> > >
> > > /* Rx descriptor*/
> > > struct vnt_rx_desc {
> > > - volatile struct vnt_rdes0 rd0;
> > > + struct vnt_rdes0 rd0;
> >
> > You can not just remove this without describing _WHY_ it is ok to do so.
> >
> > Have you properly determined why it is, or is not, ok to use volatile
> > here?
>
> I did not carefully look at the volatile usage here. After looking at it
> again, using volatile is actually valid: the structure resides on coherent
> memory.

Are you sure? That's a very odd thing for a driver to need. Looks like
they are allocating some dma memory and then pointing structures on top
of that memory. Why would you need to have "volatile" markings on a
structure definition for that?

Dig into this some more please, I don't think this is correct.

thanks,

greg k-h

2022-09-13 18:13:37

by Nam Cao

[permalink] [raw]
Subject: Re: [PATCH v3 1/2] staging: vt6655: remove unnecessary volatile qualifier

On Sun, Sep 11, 2022 at 9:25 AM Greg KH <[email protected]> wrote:
>
> On Sun, Sep 11, 2022 at 09:12:44AM +0200, Nam Cao wrote:
> > On Fri, Sep 9, 2022 at 8:03 PM Greg KH <[email protected]> wrote:
> > >
> > > On Fri, Sep 09, 2022 at 02:17:55PM +0200, Nam Cao wrote:
> > > > Remove volatile qualifier for the member rd0 of struct vnt_rx_desc,
> > > > because there is no reason it must be volatile.
> > > >
> > > > Signed-off-by: Nam Cao <[email protected]>
> > > > ---
> > > > drivers/staging/vt6655/desc.h | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/vt6655/desc.h b/drivers/staging/vt6655/desc.h
> > > > index 17a40c53b8ff..3f0f287b1693 100644
> > > > --- a/drivers/staging/vt6655/desc.h
> > > > +++ b/drivers/staging/vt6655/desc.h
> > > > @@ -182,7 +182,7 @@ struct vnt_rdes1 {
> > > >
> > > > /* Rx descriptor*/
> > > > struct vnt_rx_desc {
> > > > - volatile struct vnt_rdes0 rd0;
> > > > + struct vnt_rdes0 rd0;
> > >
> > > You can not just remove this without describing _WHY_ it is ok to do so.
> > >
> > > Have you properly determined why it is, or is not, ok to use volatile
> > > here?
> >
> > I did not carefully look at the volatile usage here. After looking at it
> > again, using volatile is actually valid: the structure resides on coherent
> > memory.
>
> Are you sure? That's a very odd thing for a driver to need. Looks like
> they are allocating some dma memory and then pointing structures on top
> of that memory. Why would you need to have "volatile" markings on a
> structure definition for that?

These structures are the ring buffer descriptors, which are dma-mapped and
their values may be changed by the hardware. For example, the field "owner" of
struct vnt_rdes0 is set to OWNED_BY_NIC by CPU, and then set to OWNED_BY_HOST
by hardware when new data arrives (at least that's why I can guess based on
the codes). So I think volatile is needed.

Please let me know if you think I'm wrong, because I have just recently
educated myself on DMA mapping.

Best regards,
Nam

2022-09-14 08:18:56

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH v3 1/2] staging: vt6655: remove unnecessary volatile qualifier

On Tue, Sep 13, 2022 at 06:52:12PM +0200, Nam Cao wrote:
> On Sun, Sep 11, 2022 at 9:25 AM Greg KH <[email protected]> wrote:
> >
> > On Sun, Sep 11, 2022 at 09:12:44AM +0200, Nam Cao wrote:
> > > On Fri, Sep 9, 2022 at 8:03 PM Greg KH <[email protected]> wrote:
> > > >
> > > > On Fri, Sep 09, 2022 at 02:17:55PM +0200, Nam Cao wrote:
> > > > > Remove volatile qualifier for the member rd0 of struct vnt_rx_desc,
> > > > > because there is no reason it must be volatile.
> > > > >
> > > > > Signed-off-by: Nam Cao <[email protected]>
> > > > > ---
> > > > > drivers/staging/vt6655/desc.h | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/staging/vt6655/desc.h b/drivers/staging/vt6655/desc.h
> > > > > index 17a40c53b8ff..3f0f287b1693 100644
> > > > > --- a/drivers/staging/vt6655/desc.h
> > > > > +++ b/drivers/staging/vt6655/desc.h
> > > > > @@ -182,7 +182,7 @@ struct vnt_rdes1 {
> > > > >
> > > > > /* Rx descriptor*/
> > > > > struct vnt_rx_desc {
> > > > > - volatile struct vnt_rdes0 rd0;
> > > > > + struct vnt_rdes0 rd0;
> > > >
> > > > You can not just remove this without describing _WHY_ it is ok to do so.
> > > >
> > > > Have you properly determined why it is, or is not, ok to use volatile
> > > > here?
> > >
> > > I did not carefully look at the volatile usage here. After looking at it
> > > again, using volatile is actually valid: the structure resides on coherent
> > > memory.
> >
> > Are you sure? That's a very odd thing for a driver to need. Looks like
> > they are allocating some dma memory and then pointing structures on top
> > of that memory. Why would you need to have "volatile" markings on a
> > structure definition for that?
>
> These structures are the ring buffer descriptors, which are dma-mapped and
> their values may be changed by the hardware. For example, the field "owner" of
> struct vnt_rdes0 is set to OWNED_BY_NIC by CPU, and then set to OWNED_BY_HOST
> by hardware when new data arrives (at least that's why I can guess based on
> the codes). So I think volatile is needed.
>
> Please let me know if you think I'm wrong, because I have just recently
> educated myself on DMA mapping.

With DMA mapping you shouldn't need "volatile" as it really doesn't do
what people thought it did. Other drivers don't use volatile for
accessing memory this way, the device should have notified the driver
that it is now safe to read from that memory through the DMA api.

So try looking a bit closer and also look at the compiler output before
and after your change to see if it actually did anything or not.

thanks,

greg k-h