[PATCH v3 0/2] use memset to make code clearer

[PATCH v3 0/2] use memset to make code clearer

[Nam Cao]

Re-write some code using memset to make it more obvious. Also remove an
unnecessary volatile qualifier, because compiler complains about passing
volatile pointer to memset.

V3: get rid of "volatile" entirely, instead of just casting it away.
V2: re-write commit message because previous message describes a
non-existent problem.

Nam Cao (2):
  staging: vt6655: remove unnecessary volatile qualifier
  staging: vt6655: use memset to make code clearer

 drivers/staging/vt6655/desc.h        | 2 +-
 drivers/staging/vt6655/device_main.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.25.1

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

[Nam Cao]

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 <namcaov@gmail.com>
---
 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

[PATCH v3 2/2] staging: vt6655: use memset to make code clearer

[Nam Cao]

A line of code sets the entire struct vnt_rdes0 to zero by treating it as
unsigned int. This works because sizeof(unsigned int) is equal to
sizeof(struct vnt_rdes0) (4 bytes). However it is not obvious what this
code is doing. Re-write this using memset to make the code clearer.

Signed-off-by: Nam Cao <namcaov@gmail.com>
---
 drivers/staging/vt6655/device_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
index 3397c78b975a..34a65a92d602 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -867,7 +867,7 @@ static bool device_alloc_rx_buf(struct vnt_private *priv,
 		return false;
 	}
 
-	*((unsigned int *)&rd->rd0) = 0; /* FIX cast */
+	memset(&rd->rd0, 0, sizeof(rd->rd0));
 
 	rd->rd0.res_count = cpu_to_le16(priv->rx_buf_sz);
 	rd->rd0.owner = OWNED_BY_NIC;
-- 
2.25.1

Re: [PATCH v3 0/2] use memset to make code clearer

[Philipp Hortmann]

On 9/9/22 14:17, Nam Cao wrote:
> Re-write some code using memset to make it more obvious. Also remove an
> unnecessary volatile qualifier, because compiler complains about passing
> volatile pointer to memset.
> 
> V3: get rid of "volatile" entirely, instead of just casting it away.
> V2: re-write commit message because previous message describes a
> non-existent problem.
> 
> Nam Cao (2):
>    staging: vt6655: remove unnecessary volatile qualifier
>    staging: vt6655: use memset to make code clearer
> 
>   drivers/staging/vt6655/desc.h        | 2 +-
>   drivers/staging/vt6655/device_main.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 

Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com>

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

[Greg KH]

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 <namcaov@gmail.com>
> ---
>  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

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

[Nam Cao]

On Fri, Sep 9, 2022 at 8:03 PM Greg KH <gregkh@linuxfoundation.org> 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 <namcaov@gmail.com>
> > ---
> >  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

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

[Greg KH]

On Sun, Sep 11, 2022 at 09:12:44AM +0200, Nam Cao wrote:
> On Fri, Sep 9, 2022 at 8:03 PM Greg KH <gregkh@linuxfoundation.org> 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 <namcaov@gmail.com>
> > > ---
> > >  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

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

[Nam Cao]

On Sun, Sep 11, 2022 at 9:25 AM Greg KH <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> 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 <namcaov@gmail.com>
> > > > ---
> > > >  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

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

[Greg KH]

On Tue, Sep 13, 2022 at 06:52:12PM +0200, Nam Cao wrote:
> On Sun, Sep 11, 2022 at 9:25 AM Greg KH <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> 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 <namcaov@gmail.com>
> > > > > ---
> > > > >  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