* [PATCH v3 0/2] use memset to make code clearer
@ 2022-09-09 12:17 Nam Cao
2022-09-09 12:17 ` [PATCH v3 1/2] staging: vt6655: remove unnecessary volatile qualifier Nam Cao
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Nam Cao @ 2022-09-09 12:17 UTC (permalink / raw)
To: forest, gregkh; +Cc: namcaov, linux-kernel, linux-staging
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v3 1/2] staging: vt6655: remove unnecessary volatile qualifier 2022-09-09 12:17 [PATCH v3 0/2] use memset to make code clearer Nam Cao @ 2022-09-09 12:17 ` Nam Cao 2022-09-09 18:03 ` Greg KH 2022-09-09 12:17 ` [PATCH v3 2/2] staging: vt6655: use memset to make code clearer Nam Cao 2022-09-09 17:58 ` [PATCH v3 0/2] " Philipp Hortmann 2 siblings, 1 reply; 9+ messages in thread From: Nam Cao @ 2022-09-09 12:17 UTC (permalink / raw) To: forest, gregkh; +Cc: namcaov, linux-kernel, linux-staging 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] staging: vt6655: remove unnecessary volatile qualifier 2022-09-09 12:17 ` [PATCH v3 1/2] staging: vt6655: remove unnecessary volatile qualifier Nam Cao @ 2022-09-09 18:03 ` Greg KH 2022-09-11 7:12 ` Nam Cao 0 siblings, 1 reply; 9+ messages in thread From: Greg KH @ 2022-09-09 18:03 UTC (permalink / raw) To: Nam Cao; +Cc: forest, linux-kernel, linux-staging 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] staging: vt6655: remove unnecessary volatile qualifier 2022-09-09 18:03 ` Greg KH @ 2022-09-11 7:12 ` Nam Cao 2022-09-11 7:25 ` Greg KH 0 siblings, 1 reply; 9+ messages in thread From: Nam Cao @ 2022-09-11 7:12 UTC (permalink / raw) To: Greg KH; +Cc: forest, linux-kernel, linux-staging 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] staging: vt6655: remove unnecessary volatile qualifier 2022-09-11 7:12 ` Nam Cao @ 2022-09-11 7:25 ` Greg KH 2022-09-13 16:52 ` Nam Cao 0 siblings, 1 reply; 9+ messages in thread From: Greg KH @ 2022-09-11 7:25 UTC (permalink / raw) To: Nam Cao; +Cc: forest, linux-kernel, linux-staging 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] staging: vt6655: remove unnecessary volatile qualifier 2022-09-11 7:25 ` Greg KH @ 2022-09-13 16:52 ` Nam Cao 2022-09-14 8:02 ` Greg KH 0 siblings, 1 reply; 9+ messages in thread From: Nam Cao @ 2022-09-13 16:52 UTC (permalink / raw) To: Greg KH; +Cc: forest, linux-kernel, linux-staging 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] staging: vt6655: remove unnecessary volatile qualifier 2022-09-13 16:52 ` Nam Cao @ 2022-09-14 8:02 ` Greg KH 0 siblings, 0 replies; 9+ messages in thread From: Greg KH @ 2022-09-14 8:02 UTC (permalink / raw) To: Nam Cao; +Cc: forest, linux-kernel, linux-staging 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 2/2] staging: vt6655: use memset to make code clearer 2022-09-09 12:17 [PATCH v3 0/2] use memset to make code clearer Nam Cao 2022-09-09 12:17 ` [PATCH v3 1/2] staging: vt6655: remove unnecessary volatile qualifier Nam Cao @ 2022-09-09 12:17 ` Nam Cao 2022-09-09 17:58 ` [PATCH v3 0/2] " Philipp Hortmann 2 siblings, 0 replies; 9+ messages in thread From: Nam Cao @ 2022-09-09 12:17 UTC (permalink / raw) To: forest, gregkh; +Cc: namcaov, linux-kernel, linux-staging 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 0/2] use memset to make code clearer 2022-09-09 12:17 [PATCH v3 0/2] use memset to make code clearer Nam Cao 2022-09-09 12:17 ` [PATCH v3 1/2] staging: vt6655: remove unnecessary volatile qualifier Nam Cao 2022-09-09 12:17 ` [PATCH v3 2/2] staging: vt6655: use memset to make code clearer Nam Cao @ 2022-09-09 17:58 ` Philipp Hortmann 2 siblings, 0 replies; 9+ messages in thread From: Philipp Hortmann @ 2022-09-09 17:58 UTC (permalink / raw) To: Nam Cao, forest, gregkh; +Cc: linux-kernel, linux-staging 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> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-09-14 8:02 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-09 12:17 [PATCH v3 0/2] use memset to make code clearer Nam Cao 2022-09-09 12:17 ` [PATCH v3 1/2] staging: vt6655: remove unnecessary volatile qualifier Nam Cao 2022-09-09 18:03 ` Greg KH 2022-09-11 7:12 ` Nam Cao 2022-09-11 7:25 ` Greg KH 2022-09-13 16:52 ` Nam Cao 2022-09-14 8:02 ` Greg KH 2022-09-09 12:17 ` [PATCH v3 2/2] staging: vt6655: use memset to make code clearer Nam Cao 2022-09-09 17:58 ` [PATCH v3 0/2] " Philipp Hortmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).