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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > > > > ---
> > > > >  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