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 <[email protected]>
---
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
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 <[email protected]>
Could you provide an example of which accesses make it problematic?
Is this fixing an actual bug?
> Lorenzo Bianconi <[email protected]> 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 <[email protected]>
>>>
>>> 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
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 <[email protected]>
> >
> > 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...
Lorenzo Bianconi <[email protected]> writes:
>> Lorenzo Bianconi <[email protected]> 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 <[email protected]>
>>>>
>>>> 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
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?
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 <[email protected]>
> > >
> > > 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
> 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 <[email protected]>
>
> 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
Lorenzo Bianconi <[email protected]> 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 <[email protected]>
>>
>> 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