diff mbox

[v2] mt7601u: make write with mask access atomic

Message ID 4a8ec536fda4741ef88618493d0641fbd497314c.1518819577.git.lorenzo.bianconi@redhat.com (mailing list archive)
State Accepted
Commit fee05843801c37e527dbe2c5eeb3fb3b15bc9919
Delegated to: Kalle Valo
Headers show

Commit Message

Lorenzo Bianconi Feb. 16, 2018, 10:30 p.m. UTC 
Introduce __mt7601u_rr and __mt7601u_vendor_single_wr routines in order
to make mt7601u_rmw and mt7601u_rmc atomic. This patch does not fix a
reported issue but makes the usb access more robust to concurrent
operations on the same register since it is theoretically possible that
read and write accesses of mt7601u_rmw/mt7601u_rmc can be interleaved with
a different write operation on the same register.
Moreover using __mt7601u_rr and __mt7601u_vendor_single_wr in
mt7601u_rmw/mt7601u_rmc allows to grab vendor_req_mutex mutex once
instead of twice

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
Changes since v1:
- improve commit log
---
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h |  3 +-
 drivers/net/wireless/mediatek/mt7601u/usb.c     | 52 ++++++++++++++++++-------
 2 files changed, 39 insertions(+), 16 deletions(-)

Comments

Jakub Kicinski Feb. 16, 2018, 10:38 p.m. UTC | #1 
On Fri, 16 Feb 2018 23:30:01 +0100, Lorenzo Bianconi wrote:
> Introduce __mt7601u_rr and __mt7601u_vendor_single_wr routines in order
> to make mt7601u_rmw and mt7601u_rmc atomic. This patch does not fix a
> reported issue but makes the usb access more robust to concurrent
> operations on the same register since it is theoretically possible that
> read and write accesses of mt7601u_rmw/mt7601u_rmc can be interleaved with
> a different write operation on the same register.
> Moreover using __mt7601u_rr and __mt7601u_vendor_single_wr in
> mt7601u_rmw/mt7601u_rmc allows to grab vendor_req_mutex mutex once
> instead of twice
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

Acked-by: Jakub Kicinski <kubakici@wp.pl>

Thanks!
Kalle Valo Feb. 28, 2018, 2:59 p.m. UTC | #2 
Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote:

> Introduce __mt7601u_rr and __mt7601u_vendor_single_wr routines in order
> to make mt7601u_rmw and mt7601u_rmc atomic. This patch does not fix a
> reported issue but makes the usb access more robust to concurrent
> operations on the same register since it is theoretically possible that
> read and write accesses of mt7601u_rmw/mt7601u_rmc can be interleaved with
> a different write operation on the same register.
> Moreover using __mt7601u_rr and __mt7601u_vendor_single_wr in
> mt7601u_rmw/mt7601u_rmc allows to grab vendor_req_mutex mutex once
> instead of twice
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> Acked-by: Jakub Kicinski <kubakici@wp.pl>

Patch applied to wireless-drivers-next.git, thanks.

fee05843801c mt7601u: make write with mask access atomic
diff mbox

Patch

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;
 }