[PATCH] mt7601u: make write with mask access atomic

[PATCH] mt7601u: make write with mask access atomic

[Lorenzo Bianconi]

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>
---
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h |  3 +-
 drivers/net/wireless/mediatek/mt7601u/usb.c     | 52 ++++++++++++++++++-------
 2 files changed, 39 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt7601u/mt7601u.h b/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
index c7ec40475a5f..9233744451a9 100644
--- a/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
+++ b/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
@@ -147,7 +147,8 @@ enum {
  * @rx_lock:		protects @rx_q.
  * @con_mon_lock:	protects @ap_bssid, @bcn_*, @avg_rssi.
  * @mutex:		ensures exclusive access from mac80211 callbacks.
- * @vendor_req_mutex:	protects @vend_buf, ensures atomicity of split writes.
+ * @vendor_req_mutex:	protects @vend_buf, ensures atomicity of read/write
+ *			accesses
  * @reg_atomic_mutex:	ensures atomicity of indirect register accesses
  *			(accesses to RF and BBP).
  * @hw_atomic_mutex:	ensures exclusive access to HW during critical
diff --git a/drivers/net/wireless/mediatek/mt7601u/usb.c b/drivers/net/wireless/mediatek/mt7601u/usb.c
index b9e4f6793138..d8b7863f7926 100644
--- a/drivers/net/wireless/mediatek/mt7601u/usb.c
+++ b/drivers/net/wireless/mediatek/mt7601u/usb.c
@@ -129,15 +129,14 @@ void mt7601u_vendor_reset(struct mt7601u_dev *dev)
 			       MT_VEND_DEV_MODE_RESET, 0, NULL, 0);
 }
 
-u32 mt7601u_rr(struct mt7601u_dev *dev, u32 offset)
+/* should be called with vendor_req_mutex held */
+static u32 __mt7601u_rr(struct mt7601u_dev *dev, u32 offset)
 {
 	int ret;
 	u32 val = ~0;
 
 	WARN_ONCE(offset > USHRT_MAX, "read high off:%08x", offset);
 
-	mutex_lock(&dev->vendor_req_mutex);
-
 	ret = mt7601u_vendor_request(dev, MT_VEND_MULTI_READ, USB_DIR_IN,
 				     0, offset, dev->vend_buf, MT_VEND_BUF);
 	if (ret == MT_VEND_BUF)
@@ -146,25 +145,41 @@ u32 mt7601u_rr(struct mt7601u_dev *dev, u32 offset)
 		dev_err(dev->dev, "Error: wrong size read:%d off:%08x\n",
 			ret, offset);
 
-	mutex_unlock(&dev->vendor_req_mutex);
-
 	trace_reg_read(dev, offset, val);
 	return val;
 }
 
-int mt7601u_vendor_single_wr(struct mt7601u_dev *dev, const u8 req,
-			     const u16 offset, const u32 val)
+u32 mt7601u_rr(struct mt7601u_dev *dev, u32 offset)
 {
-	int ret;
+	u32 ret;
 
 	mutex_lock(&dev->vendor_req_mutex);
+	ret = __mt7601u_rr(dev, offset);
+	mutex_unlock(&dev->vendor_req_mutex);
 
-	ret = mt7601u_vendor_request(dev, req, USB_DIR_OUT,
-				     val & 0xffff, offset, NULL, 0);
+	return ret;
+}
+
+/* should be called with vendor_req_mutex held */
+static int __mt7601u_vendor_single_wr(struct mt7601u_dev *dev, const u8 req,
+				      const u16 offset, const u32 val)
+{
+	int ret = mt7601u_vendor_request(dev, req, USB_DIR_OUT,
+					 val & 0xffff, offset, NULL, 0);
 	if (!ret)
 		ret = mt7601u_vendor_request(dev, req, USB_DIR_OUT,
 					     val >> 16, offset + 2, NULL, 0);
+	trace_reg_write(dev, offset, val);
+	return ret;
+}
+
+int mt7601u_vendor_single_wr(struct mt7601u_dev *dev, const u8 req,
+			     const u16 offset, const u32 val)
+{
+	int ret;
 
+	mutex_lock(&dev->vendor_req_mutex);
+	ret = __mt7601u_vendor_single_wr(dev, req, offset, val);
 	mutex_unlock(&dev->vendor_req_mutex);
 
 	return ret;
@@ -175,23 +190,30 @@ void mt7601u_wr(struct mt7601u_dev *dev, u32 offset, u32 val)
 	WARN_ONCE(offset > USHRT_MAX, "write high off:%08x", offset);
 
 	mt7601u_vendor_single_wr(dev, MT_VEND_WRITE, offset, val);
-	trace_reg_write(dev, offset, val);
 }
 
 u32 mt7601u_rmw(struct mt7601u_dev *dev, u32 offset, u32 mask, u32 val)
 {
-	val |= mt7601u_rr(dev, offset) & ~mask;
-	mt7601u_wr(dev, offset, val);
+	mutex_lock(&dev->vendor_req_mutex);
+	val |= __mt7601u_rr(dev, offset) & ~mask;
+	__mt7601u_vendor_single_wr(dev, MT_VEND_WRITE, offset, val);
+	mutex_unlock(&dev->vendor_req_mutex);
+
 	return val;
 }
 
 u32 mt7601u_rmc(struct mt7601u_dev *dev, u32 offset, u32 mask, u32 val)
 {
-	u32 reg = mt7601u_rr(dev, offset);
+	u32 reg;
 
+	mutex_lock(&dev->vendor_req_mutex);
+	reg = __mt7601u_rr(dev, offset);
 	val |= reg & ~mask;
 	if (reg != val)
-		mt7601u_wr(dev, offset, val);
+		__mt7601u_vendor_single_wr(dev, MT_VEND_WRITE,
+					   offset, val);
+	mutex_unlock(&dev->vendor_req_mutex);
+
 	return val;
 }
 
-- 
2.14.3

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

[Jakub Kicinski]

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?

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

[Lorenzo Bianconi]

> 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.

Regards,
Lorenzo

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

[Jakub Kicinski]

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.

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

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

[Lorenzo Bianconi]

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

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

[Jakub Kicinski]

On Fri, 16 Feb 2018 10:06:49 +0100, Lorenzo Bianconi wrote:
> > 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

You make good points :)  Could you please respin stating clearly in the
commit message that this is not a bug fix, but a micro-optimization and
may be useful in the future?

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

[Kalle Valo]

Lorenzo Bianconi <lorenzo.bianconi@redhat.com> writes:

>> 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?
>
> it is not an issue I had experimented, I noticed a theoretical race
> reviewing the code.

The commit log should always answer the question "Why?". If you find a
theoretical issue in the code document that clearly in the commit log.
That helps to choose correct tree and sending to stable releases etc.

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#commit_log_does_not_answer_why

-- 
Kalle Valo

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

[Lorenzo Bianconi]

> Lorenzo Bianconi <lorenzo.bianconi@redhat.com> writes:
>
>>> 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?
>>
>> it is not an issue I had experimented, I noticed a theoretical race
>> reviewing the code.
>
> The commit log should always answer the question "Why?". If you find a
> theoretical issue in the code document that clearly in the commit log.
> That helps to choose correct tree and sending to stable releases etc.
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#commit_log_does_not_answer_why
>
> --
> Kalle Valo

Hi Kalle,

I have already sent a v2: https://patchwork.kernel.org/patch/10225849/
Is the commit message ok?

Regards,
Lorenzo

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

[Kalle Valo]

Lorenzo Bianconi <lorenzo.bianconi@redhat.com> writes:

>> Lorenzo Bianconi <lorenzo.bianconi@redhat.com> writes:
>>
>>>> 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?
>>>
>>> it is not an issue I had experimented, I noticed a theoretical race
>>> reviewing the code.
>>
>> The commit log should always answer the question "Why?". If you find a
>> theoretical issue in the code document that clearly in the commit log.
>> That helps to choose correct tree and sending to stable releases etc.
>>
>> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#commit_log_does_not_answer_why
>
> I have already sent a v2: https://patchwork.kernel.org/patch/10225849/
> Is the commit message ok?

Yeah, I already applied it.

-- 
Kalle Valo