Re: [PATCH] mt7601u: make write with mask access atomic

[Lorenzo Bianconi <lorenzo.bianconi@redhat.com>]

On Feb 15, Jakub Kicinski wrote:
> On Fri, 16 Feb 2018 01:02:31 +0100, Lorenzo Bianconi wrote:
> > > On Thu, 15 Feb 2018 23:59:24 +0100, Lorenzo Bianconi wrote:  
> > >> Introduce __mt7601u_rr and __mt7601u_vendor_single_wr routines in order
> > >> to make mt7601u_rmw and mt7601u_rmc atomic since it is possible that
> > >> read and write accesses of mt7601u_rmw/mt7601u_rmc can be interleaved
> > >> with a different write operation on the same register.
> > >> Moreover move write trace point in __mt7601u_vendor_single_wr
> > >>
> > >> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>  
> > >
> > > Could you provide an example of which accesses make it problematic?
> > > Is this fixing an actual bug?  
> > 
> > Hi Jakub,
> > 
> > it is not an issue I had experimented, I noticed a theoretical race
> > reviewing the code.
> > AFAIU, based on the current implementation it is possible that mt7601u_rmw
> > (with mt7601u_rr) loads data from given register but its store access
> > (mt7601u_wr) is
> > preceded by another mt7601u_wr on the same register. In this case the
> > value configured by
> > the first mt7601u_wr executed is overwritten by the second one (the
> > store from mt7601u_rmw)
> > even if the first write is setting a different subfield respect to
> > mt7601u_rmw.
> 
> Hm.. There should be no path in the driver where that could cause
> problems AFAIR.  We have reg_atomic_mutex and hw_atomic_mutex to
> protect from concurrent access to the HW where they are possible.
> RMW operations are non-atomic by definition, it's supposed to work 
> like PCIe register accesses would - 32bit reads/writes are atomic, 
> but RMW is not.

Yes, RMW accesses are non-atomic by default but since vendor_req_mutex mutex
is already there (and grabbed for RMW operations), why not use it to make write
with mask access atomic without adding complexity? Moreover it would be a
micro-optimisation since vendor_req_mutex would be grabbed just once instead of
twice

> 
> So I'm not sure what to do with this patch.  Doesn't seem necessary...

It is just a trivial rework of locking in usb read/write accesses, not
mandatory, so if you prefer we can just drop it

Regards,
Lorenzo