Re: [PATCH v4] Staging: iio: ade7758: Expand buf_lock to cover both buffer and state protection
On Sat, 3 Feb 2018 21:01:32 +0530
Shreeya Patel wrote:
> iio_dev->mlock is to be used only by the IIO core for protecting
> device mode changes between INDIO_DIRECT and INDIO_BUFFER.
>
> This patch replaces the use of mlock with the already established
> buf_lock mutex.
>
> Introducing 'unlocked' forms of read and write registers. The
> read/write frequency functions now require buf_lock to be held.
> That's not obvious so avoid this but moving the locking inside
> the functions where it is then clear that they are taking the
> unlocked forms of the register read/write.
>
> It isn't readily apparent that write frequency function requires
> the locks to be taken, so move it inside the function to where it
> is required to protect.
>
> Signed-off-by: Shreeya Patel
Hi Shreeya,
Unfortunately this introduces a new bug - you end up unlocking
a mutex that you never locked in one of the error paths.
See inline.
We are also still taking the mlock around the read of the
frequency which doesn't make any sense given there is
no reason to protect that against state changes.
Arguably fixing that could be a separate patch as it never
made much sense, but it should probably be in this same series
at least. I would have no real problem with it being in it this
same patch as long as the description above mentions it.
Thanks,
Jonathan
> ---
>
> Changes in v2
> -Add static keyword to newly introduced functions and remove some
> added comments which are not required.
>
> Changes in v3
> -Remove some useless mlocks and send it as another patch.
> Also make the necessary change in the current patch associated with
> the new patch with commit id 88eba33. Make commit message more
> appropriate.
>
> Changes in v4
> -Write frequency function do not require lock so move it inside
> the function to where it is required to protect.
>
>
> drivers/staging/iio/meter/ade7758.h | 2 +-
> drivers/staging/iio/meter/ade7758_core.c | 49
>
> 2 files changed, 38 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/iio/meter/ade7758.h
> b/drivers/staging/iio/meter/ade7758.h
> index 6ae78d8..2de81b5 100644
> --- a/drivers/staging/iio/meter/ade7758.h
> +++ b/drivers/staging/iio/meter/ade7758.h
> @@ -111,7 +111,7 @@
> * @trig:data ready trigger registered with iio
> * @tx: transmit buffer
> * @rx: receive buffer
> - * @buf_lock:mutex to protect tx and rx
> + * @buf_lock:mutex to protect tx, rx, read and write
> frequency
> **/
> struct ade7758_state {
> struct spi_device *us;
> diff --git a/drivers/staging/iio/meter/ade7758_core.c
> b/drivers/staging/iio/meter/ade7758_core.c
> index 227dbfc..ff19d46 100644
> --- a/drivers/staging/iio/meter/ade7758_core.c
> +++ b/drivers/staging/iio/meter/ade7758_core.c
> @@ -24,17 +24,25 @@
> #include "meter.h"
> #include "ade7758.h"
>
> -int ade7758_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
> +static int __ade7758_spi_write_reg_8(struct device *dev, u8 reg_address, u8
> val)
> {
> - int ret;
> struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> struct ade7758_state *st = iio_priv(indio_dev);
>
> - mutex_lock(&st->buf_lock);
> st->tx[0] = ADE7758_WRITE_REG(reg_address);
> st->tx[1] = val;
>
> - ret = spi_write(st->us, st->tx, 2);
> + return spi_write(st->us, st->tx, 2);
> +}
> +
> +int ade7758_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
> +{
> + int ret;
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct ade7758_state *st = iio_priv(indio_dev);
> +
> + mutex_lock(&st->buf_lock);
> + ret = __ade7758_spi_write_reg_8(dev, reg_address, val);
> mutex_unlock(&st->buf_lock);
>
> return ret;
> @@ -91,7 +99,7 @@ static int ade7758_spi_write_reg_24(struct device *dev, u8
> reg_address,
> return ret;
> }
>
> -int ade7758_spi_read_reg_8(struct device *dev, u8 reg_address, u8 *val)
> +static int __ade7758_spi_read_reg_8(struct device *dev, u8 reg_address, u8
> *val)
> {
> struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> struct ade7758_state *st = iio_priv(indio_dev);
> @@ -111,7 +119,6 @@ int ade7758_spi_read_reg_8(struct device *dev, u8
> reg_address, u8 *val)
> },
> };
>
> - mutex_lock(&st->buf_lock);
> st->tx[0] = ADE7758_READ_REG(reg_address);
> st->tx[1] = 0;
>
> @@ -124,7 +131,19 @@ int ade7758_spi_read_reg_8(struct device *dev, u8
> reg_address, u8 *val)
> *val = st->rx[0];
>
> error_ret:
> + return ret;
> +}
> +
> +int ade7758_spi_read_reg_8(struct device *dev, u8 reg_address, u8 *val)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct ade7758_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(&st->buf_lock);
> + ret = __ade7758_spi_read_reg_8(dev, reg_address, val);
> m
Re: [PATCH] staging: lustre: llite: replace variable length array
On 01/30/2018 03:04 AM, Dilger, Andreas wrote:
> On Jan 27, 2018, at 14:42, Sven Dziadek wrote:
>>
>> The functionality of the removed variable length array is already
>> implemented by the function xattr_full_name in fs/xattr.c
>>
>> This fixes the sparse warning:
>> warning: Variable length array is used.
>>
>> Signed-off-by: Sven Dziadek
>> ---
>> drivers/staging/lustre/lustre/llite/xattr.c | 12
>> 1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/staging/lustre/lustre/llite/xattr.c
>> b/drivers/staging/lustre/lustre/llite/xattr.c
>> index 532384c91447..4fd28213c6a1 100644
>> --- a/drivers/staging/lustre/lustre/llite/xattr.c
>> +++ b/drivers/staging/lustre/lustre/llite/xattr.c
>> @@ -87,7 +87,6 @@ ll_xattr_set_common(const struct xattr_handler *handler,
>> const char *name, const void *value, size_t size,
>> int flags)
>> {
>> -char fullname[strlen(handler->prefix) + strlen(name) + 1];
>> struct ll_sb_info *sbi = ll_i2sbi(inode);
>> struct ptlrpc_request *req = NULL;
>> const char *pv = value;
>> @@ -141,9 +140,8 @@ ll_xattr_set_common(const struct xattr_handler *handler,
>> return -EPERM;
>> }
>>
>> -sprintf(fullname, "%s%s\n", handler->prefix, name);
>> -rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
>> - valid, fullname, pv, size, 0, flags,
>> +rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode), valid,
>> + xattr_full_name(handler, name), pv, size, 0, flags,
>> ll_i2suppgid(inode), &req);
>
> Hi Sven,
> thanks for the patch.
>
> Looking at the details of "xattr_full_name()", this seems quite risky. This
> is essentially returning the pointer _before_ "name" on the assumption that
> it contains the full "prefix.name" string. IMHO, that is not necessarily a
> safe assumption to make several layers down in the code.
Thanks for your reply. And yeah, it feels strange, right.
But it really looks like this is how the name and the prefix is handled
in the xattr code.
It seems this helper function has been introduced with commit
e409de992e3ea (which also removes some fullname pointer) and indeed,
fs/xattr.c splits the prefix and the name in xattr_resolve_name.
So I still think the patch should be correct..?
> James, I thought you had a patch for this to use kasprintf() instead of the
> on-stack "fullname" declaration?
Sorry for replying that late, I though James would maybe know if the
above assumption is correct..
>> if (rc) {
>> if (rc == -EOPNOTSUPP && handler->flags == XATTR_USER_T) {
>> @@ -364,7 +362,6 @@ static int ll_xattr_get_common(const struct
>> xattr_handler *handler,
>> struct dentry *dentry, struct inode *inode,
>> const char *name, void *buffer, size_t size)
>> {
>> -char fullname[strlen(handler->prefix) + strlen(name) + 1];
>> struct ll_sb_info *sbi = ll_i2sbi(inode);
>> #ifdef CONFIG_FS_POSIX_ACL
>> struct ll_inode_info *lli = ll_i2info(inode);
>> @@ -411,9 +408,8 @@ static int ll_xattr_get_common(const struct
>> xattr_handler *handler,
>> if (handler->flags == XATTR_ACL_DEFAULT_T && !S_ISDIR(inode->i_mode))
>> return -ENODATA;
>> #endif
>> -sprintf(fullname, "%s%s\n", handler->prefix, name);
>> -return ll_xattr_list(inode, fullname, handler->flags, buffer, size,
>> - OBD_MD_FLXATTR);
>> +return ll_xattr_list(inode, xattr_full_name(handler, name),
>> + handler->flags, buffer, size, OBD_MD_FLXATTR);
>> }
>>
>> static ssize_t ll_getxattr_lov(struct inode *inode, void *buf, size_t
>> buf_size)
>> --
>> 2.11.0
>>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 3/4] staging:r8188eu: Remove struct pkt_file from rtw_xmitframe_coalesce()
Struct pkt_file is a base to simple wrapper for skb_copy_bits().
Eliminate struct pkt_file usage in rtw_xmitframe_coalesce().
Signed-off-by: Ivan Safonov
---
drivers/staging/rtl8188eu/core/rtw_xmit.c | 17 +
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/rtl8188eu/core/rtw_xmit.c
b/drivers/staging/rtl8188eu/core/rtw_xmit.c
index eb46e34ba562..6b32c142fdcc 100644
--- a/drivers/staging/rtl8188eu/core/rtw_xmit.c
+++ b/drivers/staging/rtl8188eu/core/rtw_xmit.c
@@ -943,7 +943,6 @@ This sub-routine will perform all the following:
*/
s32 rtw_xmitframe_coalesce(struct adapter *padapter, struct sk_buff *pkt,
struct xmit_frame *pxmitframe)
{
- struct pkt_file pktfile;
s32 frg_inx, frg_len, mpdu_len, llc_sz, mem_sz;
size_t addr;
u8 *pframe, *mem_start;
@@ -954,7 +953,7 @@ s32 rtw_xmitframe_coalesce(struct adapter *padapter, struct
sk_buff *pkt, struct
u8 *pbuf_start;
s32 bmcst = IS_MCAST(pattrib->ra);
s32 res = _SUCCESS;
-
+ size_t remainder = pkt->len - ETH_HLEN;
psta = rtw_get_stainfo(&padapter->stapriv, pattrib->ra);
@@ -979,9 +978,6 @@ s32 rtw_xmitframe_coalesce(struct adapter *padapter, struct
sk_buff *pkt, struct
goto exit;
}
- _rtw_open_pktfile(pkt, &pktfile);
- _rtw_pktfile_read(&pktfile, NULL, ETH_HLEN);
-
frg_inx = 0;
frg_len = pxmitpriv->frag_len - 4;/* 2346-4 = 2342 */
@@ -1038,12 +1034,9 @@ s32 rtw_xmitframe_coalesce(struct adapter *padapter,
struct sk_buff *pkt, struct
if ((pattrib->icv_len > 0) && (pattrib->bswenc))
mpdu_len -= pattrib->icv_len;
- if (bmcst) {
- /* don't do fragment to broadcat/multicast packets */
- mem_sz = _rtw_pktfile_read(&pktfile, pframe,
pattrib->pktlen);
- } else {
- mem_sz = _rtw_pktfile_read(&pktfile, pframe, mpdu_len);
- }
+ mem_sz = min_t(size_t, bmcst ? pattrib->pktlen : mpdu_len,
remainder);
+ skb_copy_bits(pkt, pkt->len - remainder, pframe, mem_sz);
+ remainder -= mem_sz;
pframe += mem_sz;
@@ -1054,7 +1047,7 @@ s32 rtw_xmitframe_coalesce(struct adapter *padapter,
struct sk_buff *pkt, struct
frg_inx++;
- if (bmcst || pktfile.pkt_len == 0) {
+ if (bmcst || remainder == 0) {
pattrib->nr_frags = frg_inx;
pattrib->last_txcmdsz = pattrib->hdrlen +
pattrib->iv_len + ((pattrib->nr_frags == 1) ? llc_sz : 0) +
--
2.13.6
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/4] staging:r8188eu: Remove struct pkt_file from set_qos()
Struct pkt_file is a base to simple wrapper for skb_copy_bits().
Use skb_copy_bits() without wrappers.
Signed-off-by: Ivan Safonov
---
drivers/staging/rtl8188eu/core/rtw_xmit.c | 26 ++
1 file changed, 10 insertions(+), 16 deletions(-)
diff --git a/drivers/staging/rtl8188eu/core/rtw_xmit.c
b/drivers/staging/rtl8188eu/core/rtw_xmit.c
index e8d9858f2942..76b652648d3d 100644
--- a/drivers/staging/rtl8188eu/core/rtw_xmit.c
+++ b/drivers/staging/rtl8188eu/core/rtw_xmit.c
@@ -387,27 +387,21 @@ u8qos_acm(u8 acm_mask, u8 priority)
return change_priority;
}
-static void set_qos(struct pkt_file *ppktfile, struct pkt_attrib *pattrib)
+static void set_qos(struct sk_buff *skb, struct pkt_attrib *pattrib)
{
- struct ethhdr etherhdr;
- struct iphdr ip_hdr;
- s32 user_prio = 0;
-
- _rtw_open_pktfile(ppktfile->pkt, ppktfile);
- _rtw_pktfile_read(ppktfile, (unsigned char *)ðerhdr, ETH_HLEN);
-
- /* get user_prio from IP hdr */
if (pattrib->ether_type == 0x0800) {
- _rtw_pktfile_read(ppktfile, (u8 *)&ip_hdr, sizeof(ip_hdr));
-/* user_prio = (ntohs(ip_hdr.tos) >> 5) & 0x3; */
- user_prio = ip_hdr.tos >> 5;
+ struct iphdr ip_hdr;
+
+ skb_copy_bits(skb, ETH_HLEN, &ip_hdr, sizeof(ip_hdr));
+ pattrib->priority = ip_hdr.tos >> 5;
} else if (pattrib->ether_type == ETH_P_PAE) {
/* "When priority processing of data frames is supported, */
/* a STA's SME should send EAPOL-Key frames at the highest
priority." */
- user_prio = 7;
+ pattrib->priority = 7;
+ } else {
+ pattrib->priority = 0;
}
- pattrib->priority = user_prio;
pattrib->hdrlen = WLAN_HDR_A3_QOS_LEN;
pattrib->subtype = WIFI_QOS_DATA_TYPE;
}
@@ -516,10 +510,10 @@ static s32 update_attrib(struct adapter *padapter, struct
sk_buff *pkt, struct p
if (check_fwstate(pmlmepriv,
WIFI_AP_STATE|WIFI_ADHOC_STATE|WIFI_ADHOC_MASTER_STATE)) {
if (psta->qos_option)
- set_qos(&pktfile, pattrib);
+ set_qos(pktfile.pkt, pattrib);
} else {
if (pqospriv->qos_option) {
- set_qos(&pktfile, pattrib);
+ set_qos(pktfile.pkt, pattrib);
if (pmlmepriv->acm_mask != 0)
pattrib->priority =
qos_acm(pmlmepriv->acm_mask, pattrib->priority);
--
2.13.6
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/4] staging:r8188eu: Remove struct pkt_file from update_attrib()
Struct pkt_file is a base to simple wrapper for skb_copy_bits().
Do not use struct pkt_file in update_attrib().
Signed-off-by: Ivan Safonov
---
drivers/staging/rtl8188eu/core/rtw_xmit.c | 16 +++-
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/rtl8188eu/core/rtw_xmit.c
b/drivers/staging/rtl8188eu/core/rtw_xmit.c
index 76b652648d3d..eb46e34ba562 100644
--- a/drivers/staging/rtl8188eu/core/rtw_xmit.c
+++ b/drivers/staging/rtl8188eu/core/rtw_xmit.c
@@ -408,7 +408,6 @@ static void set_qos(struct sk_buff *skb, struct pkt_attrib
*pattrib)
static s32 update_attrib(struct adapter *padapter, struct sk_buff *pkt, struct
pkt_attrib *pattrib)
{
- struct pkt_file pktfile;
struct sta_info *psta = NULL;
struct ethhdr etherhdr;
@@ -419,9 +418,7 @@ static s32 update_attrib(struct adapter *padapter, struct
sk_buff *pkt, struct p
struct qos_priv *pqospriv = &pmlmepriv->qospriv;
int res = _SUCCESS;
-
- _rtw_open_pktfile(pkt, &pktfile);
- _rtw_pktfile_read(&pktfile, (u8 *)ðerhdr, ETH_HLEN);
+ skb_copy_bits(pkt, 0, ðerhdr, ETH_HLEN);
pattrib->ether_type = ntohs(etherhdr.h_proto);
@@ -442,16 +439,17 @@ static s32 update_attrib(struct adapter *padapter, struct
sk_buff *pkt, struct p
memcpy(pattrib->ta, get_bssid(pmlmepriv), ETH_ALEN);
}
- pattrib->pktlen = pktfile.pkt_len;
+ pattrib->pktlen = pkt->len - ETH_HLEN;
if (pattrib->ether_type == ETH_P_IP) {
/* The following is for DHCP and ARP packet, we use cck1M to
tx these packets and let LPS awake some time */
/* to prevent DHCP protocol fail */
u8 tmp[24];
- _rtw_pktfile_read(&pktfile, &tmp[0], 24);
+ skb_copy_bits(pkt, ETH_HLEN, tmp, 24);
+
pattrib->dhcp_pkt = 0;
- if (pktfile.pkt_len > 282) {/* MINIMUM_DHCP_PACKET_SIZE) { */
+ if (pkt->len > ETH_HLEN + 24 + 282) {/*
MINIMUM_DHCP_PACKET_SIZE) { */
if (pattrib->ether_type == ETH_P_IP) {/* IP header */
if (((tmp[21] == 68) && (tmp[23] == 67)) ||
((tmp[21] == 67) && (tmp[23] == 68))) {
@@ -510,10 +508,10 @@ static s32 update_attrib(struct adapter *padapter, struct
sk_buff *pkt, struct p
if (check_fwstate(pmlmepriv,
WIFI_AP_STATE|WIFI_ADHOC_STATE|WIFI_ADHOC_MASTER_STATE)) {
if (psta->qos_option)
- set_qos(pktfile.pkt, pattrib);
+ set_qos(pkt, pattrib);
} else {
if (pqospriv->qos_option) {
- set_qos(pktfile.pkt, pattrib);
+ set_qos(pkt, pattrib);
if (pmlmepriv->acm_mask != 0)
pattrib->priority =
qos_acm(pmlmepriv->acm_mask, pattrib->priority);
--
2.13.6
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 4/4] staging:r8188eu: Remove unused struct pkt_file
Struct pkt_file is unused now, so remove it and correponding functions.
Signed-off-by: Ivan Safonov
---
drivers/staging/rtl8188eu/include/xmit_osdep.h | 13 -
drivers/staging/rtl8188eu/os_dep/xmit_linux.c | 37 --
2 files changed, 50 deletions(-)
diff --git a/drivers/staging/rtl8188eu/include/xmit_osdep.h
b/drivers/staging/rtl8188eu/include/xmit_osdep.h
index 959ef4b3066c..00ebad88f0d1 100644
--- a/drivers/staging/rtl8188eu/include/xmit_osdep.h
+++ b/drivers/staging/rtl8188eu/include/xmit_osdep.h
@@ -18,15 +18,6 @@
#include
#include
-struct pkt_file {
- struct sk_buff *pkt;
- size_t pkt_len; /* the remainder length of the open_file */
- unsigned char *cur_buffer;
- u8 *buf_start;
- u8 *cur_addr;
- size_t buf_len;
-};
-
#define NR_XMITFRAME 256
struct xmit_priv;
@@ -43,10 +34,6 @@ int rtw_os_xmit_resource_alloc(struct adapter *padapter,
struct xmit_buf *pxmitbuf, u32 alloc_sz);
void rtw_os_xmit_resource_free(struct xmit_buf *pxmitbuf);
-uint rtw_remainder_len(struct pkt_file *pfile);
-void _rtw_open_pktfile(struct sk_buff *pkt, struct pkt_file *pfile);
-uint _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, uint rlen);
-
void rtw_os_pkt_complete(struct adapter *padapter, struct sk_buff *pkt);
void rtw_os_xmit_complete(struct adapter *padapter,
struct xmit_frame *pxframe);
diff --git a/drivers/staging/rtl8188eu/os_dep/xmit_linux.c
b/drivers/staging/rtl8188eu/os_dep/xmit_linux.c
index 8bf8248e4ac7..8ac9567c954d 100644
--- a/drivers/staging/rtl8188eu/os_dep/xmit_linux.c
+++ b/drivers/staging/rtl8188eu/os_dep/xmit_linux.c
@@ -22,43 +22,6 @@
#include
#include
-uint rtw_remainder_len(struct pkt_file *pfile)
-{
- return pfile->buf_len - ((size_t)(pfile->cur_addr) -
- (size_t)(pfile->buf_start));
-}
-
-void _rtw_open_pktfile(struct sk_buff *pktptr, struct pkt_file *pfile)
-{
-
- pfile->pkt = pktptr;
- pfile->cur_addr = pktptr->data;
- pfile->buf_start = pktptr->data;
- pfile->pkt_len = pktptr->len;
- pfile->buf_len = pktptr->len;
-
- pfile->cur_buffer = pfile->buf_start;
-
-}
-
-uint _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, uint rlen)
-{
- uintlen = 0;
-
-
- len = rtw_remainder_len(pfile);
- len = min(rlen, len);
-
- if (rmem)
- skb_copy_bits(pfile->pkt, pfile->buf_len-pfile->pkt_len, rmem,
len);
-
- pfile->cur_addr += len;
- pfile->pkt_len -= len;
-
-
- return len;
-}
-
int rtw_os_xmit_resource_alloc(struct adapter *padapter, struct xmit_buf
*pxmitbuf, u32 alloc_sz)
{
int i;
--
2.13.6
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] Staging: iio: ade7758: Expand buf_lock to cover both buffer and state protection
On Sun, 2018-02-04 at 11:10 +, Jonathan Cameron wrote:
> On Sat, 3 Feb 2018 21:01:32 +0530
> Shreeya Patel wrote:
>
> >
> > iio_dev->mlock is to be used only by the IIO core for protecting
> > device mode changes between INDIO_DIRECT and INDIO_BUFFER.
> >
> > This patch replaces the use of mlock with the already established
> > buf_lock mutex.
> >
> > Introducing 'unlocked' forms of read and write registers. The
> > read/write frequency functions now require buf_lock to be held.
> > That's not obvious so avoid this but moving the locking inside
> > the functions where it is then clear that they are taking the
> > unlocked forms of the register read/write.
> >
> > It isn't readily apparent that write frequency function requires
> > the locks to be taken, so move it inside the function to where it
> > is required to protect.
> >
> > Signed-off-by: Shreeya Patel
> Hi Shreeya,
>
Hello sir,
> Unfortunately this introduces a new bug - you end up unlocking
> a mutex that you never locked in one of the error paths.
> See inline.
I'll make this change.
>
> We are also still taking the mlock around the read of the
> frequency which doesn't make any sense given there is
> no reason to protect that against state changes.
> Arguably fixing that could be a separate patch as it never
> made much sense, but it should probably be in this same series
> at least. I would have no real problem with it being in it this
> same patch as long as the description above mentions it.
>
> Thanks,
>
> Jonathan
>
> >
> > ---
> >
> > Changes in v2
> > -Add static keyword to newly introduced functions and remove some
> > added comments which are not required.
> >
> > Changes in v3
> > -Remove some useless mlocks and send it as another patch.
> > Also make the necessary change in the current patch associated
> > with
> > the new patch with commit id 88eba33. Make commit message more
> > appropriate.
> >
> > Changes in v4
> > -Write frequency function do not require lock so move it inside
> > the function to where it is required to protect.
> >
> >
> > drivers/staging/iio/meter/ade7758.h | 2 +-
> > drivers/staging/iio/meter/ade7758_core.c | 49
> >
> > 2 files changed, 38 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/staging/iio/meter/ade7758.h
> > b/drivers/staging/iio/meter/ade7758.h
> > index 6ae78d8..2de81b5 100644
> > --- a/drivers/staging/iio/meter/ade7758.h
> > +++ b/drivers/staging/iio/meter/ade7758.h
> > @@ -111,7 +111,7 @@
> > * @trig: data ready trigger registered with iio
> > * @tx:transmit buffer
> > * @rx:receive buffer
> > - * @buf_lock: mutex to protect tx and rx
> > + * @buf_lock: mutex to protect tx, rx, read and
> > write frequency
> > **/
> > struct ade7758_state {
> > struct spi_device *us;
> > diff --git a/drivers/staging/iio/meter/ade7758_core.c
> > b/drivers/staging/iio/meter/ade7758_core.c
> > index 227dbfc..ff19d46 100644
> > --- a/drivers/staging/iio/meter/ade7758_core.c
> > +++ b/drivers/staging/iio/meter/ade7758_core.c
> > @@ -24,17 +24,25 @@
> > #include "meter.h"
> > #include "ade7758.h"
> >
> > -int ade7758_spi_write_reg_8(struct device *dev, u8 reg_address, u8
> > val)
> > +static int __ade7758_spi_write_reg_8(struct device *dev, u8
> > reg_address, u8 val)
> > {
> > - int ret;
> > struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > struct ade7758_state *st = iio_priv(indio_dev);
> >
> > - mutex_lock(&st->buf_lock);
> > st->tx[0] = ADE7758_WRITE_REG(reg_address);
> > st->tx[1] = val;
> >
> > - ret = spi_write(st->us, st->tx, 2);
> > + return spi_write(st->us, st->tx, 2);
> > +}
> > +
> > +int ade7758_spi_write_reg_8(struct device *dev, u8 reg_address, u8
> > val)
> > +{
> > + int ret;
> > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > + struct ade7758_state *st = iio_priv(indio_dev);
> > +
> > + mutex_lock(&st->buf_lock);
> > + ret = __ade7758_spi_write_reg_8(dev, reg_address, val);
> > mutex_unlock(&st->buf_lock);
> >
> > return ret;
> > @@ -91,7 +99,7 @@ static int ade7758_spi_write_reg_24(struct device
> > *dev, u8 reg_address,
> > return ret;
> > }
> >
> > -int ade7758_spi_read_reg_8(struct device *dev, u8 reg_address, u8
> > *val)
> > +static int __ade7758_spi_read_reg_8(struct device *dev, u8
> > reg_address, u8 *val)
> > {
> > struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > struct ade7758_state *st = iio_priv(indio_dev);
> > @@ -111,7 +119,6 @@ int ade7758_spi_read_reg_8(struct device *dev,
> > u8 reg_address, u8 *val)
> > },
> > };
> >
> > - mutex_lock(&st->buf_lock);
> > st->tx[0] = ADE7758_READ_REG(reg_address);
> > st->tx[1] = 0;
> >
> > @@ -124,7 +131,19 @@ int ade7758_spi_read_reg_8(struct device *dev,
> > u8 reg_address, u8 *val)
> > *val = st->rx[0];
> >
> > error_ret:
> > + return ret;
> > +}
Re: [PATCH v4] Staging: iio: ade7758: Expand buf_lock to cover both buffer and state protection
On 4 February 2018 21:03:05 GMT, Shreeya Patel
wrote:
>On Sun, 2018-02-04 at 11:10 +, Jonathan Cameron wrote:
>> On Sat, 3 Feb 2018 21:01:32 +0530
>> Shreeya Patel wrote:
>>
>> >
>> > iio_dev->mlock is to be used only by the IIO core for protecting
>> > device mode changes between INDIO_DIRECT and INDIO_BUFFER.
>> >
>> > This patch replaces the use of mlock with the already established
>> > buf_lock mutex.
>> >
>> > Introducing 'unlocked' forms of read and write registers. The
>> > read/write frequency functions now require buf_lock to be held.
>> > That's not obvious so avoid this but moving the locking inside
>> > the functions where it is then clear that they are taking the
>> > unlocked forms of the register read/write.
>> >
>> > It isn't readily apparent that write frequency function requires
>> > the locks to be taken, so move it inside the function to where it
>> > is required to protect.
>> >
>> > Signed-off-by: Shreeya Patel
>> Hi Shreeya,
>>
>Hello sir,
>
>> Unfortunately this introduces a new bug - you end up unlocking
>> a mutex that you never locked in one of the error paths.
>> See inline.
>I'll make this change.
>>
>> We are also still taking the mlock around the read of the
>> frequency which doesn't make any sense given there is
>> no reason to protect that against state changes.
>> Arguably fixing that could be a separate patch as it never
>> made much sense, but it should probably be in this same series
>> at least. I would have no real problem with it being in it this
>> same patch as long as the description above mentions it.
>>
>> Thanks,
>>
>> Jonathan
>>
>> >
>> > ---
>> >
>> > Changes in v2
>> > -Add static keyword to newly introduced functions and remove some
>> > added comments which are not required.
>> >
>> > Changes in v3
>> > -Remove some useless mlocks and send it as another patch.
>> > Also make the necessary change in the current patch associated
>> > with
>> > the new patch with commit id 88eba33. Make commit message more
>> > appropriate.
>> >
>> > Changes in v4
>> > -Write frequency function do not require lock so move it inside
>> > the function to where it is required to protect.
>> >
>> >
>> > drivers/staging/iio/meter/ade7758.h | 2 +-
>> > drivers/staging/iio/meter/ade7758_core.c | 49
>> >
>> > 2 files changed, 38 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/drivers/staging/iio/meter/ade7758.h
>> > b/drivers/staging/iio/meter/ade7758.h
>> > index 6ae78d8..2de81b5 100644
>> > --- a/drivers/staging/iio/meter/ade7758.h
>> > +++ b/drivers/staging/iio/meter/ade7758.h
>> > @@ -111,7 +111,7 @@
>> > * @trig: data ready trigger registered with iio
>> > * @tx: transmit buffer
>> > * @rx: receive buffer
>> > - * @buf_lock: mutex to protect tx and rx
>> > + * @buf_lock: mutex to protect tx, rx, read and
>> > write frequency
>> > **/
>> > struct ade7758_state {
>> > struct spi_device *us;
>> > diff --git a/drivers/staging/iio/meter/ade7758_core.c
>> > b/drivers/staging/iio/meter/ade7758_core.c
>> > index 227dbfc..ff19d46 100644
>> > --- a/drivers/staging/iio/meter/ade7758_core.c
>> > +++ b/drivers/staging/iio/meter/ade7758_core.c
>> > @@ -24,17 +24,25 @@
>> > #include "meter.h"
>> > #include "ade7758.h"
>> >
>> > -int ade7758_spi_write_reg_8(struct device *dev, u8 reg_address, u8
>> > val)
>> > +static int __ade7758_spi_write_reg_8(struct device *dev, u8
>> > reg_address, u8 val)
>> > {
>> > - int ret;
>> > struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> > struct ade7758_state *st = iio_priv(indio_dev);
>> >
>> > - mutex_lock(&st->buf_lock);
>> > st->tx[0] = ADE7758_WRITE_REG(reg_address);
>> > st->tx[1] = val;
>> >
>> > - ret = spi_write(st->us, st->tx, 2);
>> > + return spi_write(st->us, st->tx, 2);
>> > +}
>> > +
>> > +int ade7758_spi_write_reg_8(struct device *dev, u8 reg_address, u8
>> > val)
>> > +{
>> > + int ret;
>> > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> > + struct ade7758_state *st = iio_priv(indio_dev);
>> > +
>> > + mutex_lock(&st->buf_lock);
>> > + ret = __ade7758_spi_write_reg_8(dev, reg_address, val);
>> > mutex_unlock(&st->buf_lock);
>> >
>> > return ret;
>> > @@ -91,7 +99,7 @@ static int ade7758_spi_write_reg_24(struct device
>> > *dev, u8 reg_address,
>> > return ret;
>> > }
>> >
>> > -int ade7758_spi_read_reg_8(struct device *dev, u8 reg_address, u8
>> > *val)
>> > +static int __ade7758_spi_read_reg_8(struct device *dev, u8
>> > reg_address, u8 *val)
>> > {
>> > struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> > struct ade7758_state *st = iio_priv(indio_dev);
>> > @@ -111,7 +119,6 @@ int ade7758_spi_read_reg_8(struct device *dev,
>> > u8 reg_address, u8 *val)
>> > },
>> > };
>> >
>> > - mutex_lock(&st->buf_lock);
>> > st->tx[0] = ADE7758_READ_REG(reg_address);
>> > st->tx[1] = 0;
>> >
>> > @
[PATCH 2/2] staging: android: ion: Return void instead of int
Now, nobody care about the return value of ion_page_pool_add, therefore
we can just make it return void.
Signed-off-by: Yisheng Xie
---
drivers/staging/android/ion/ion_page_pool.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/android/ion/ion_page_pool.c
b/drivers/staging/android/ion/ion_page_pool.c
index 150626f..e3a6e32 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -26,7 +26,7 @@ static void ion_page_pool_free_pages(struct ion_page_pool
*pool,
__free_pages(page, pool->order);
}
-static int ion_page_pool_add(struct ion_page_pool *pool, struct page *page)
+static void ion_page_pool_add(struct ion_page_pool *pool, struct page *page)
{
mutex_lock(&pool->mutex);
if (PageHighMem(page)) {
@@ -37,7 +37,6 @@ static int ion_page_pool_add(struct ion_page_pool *pool,
struct page *page)
pool->low_count++;
}
mutex_unlock(&pool->mutex);
- return 0;
}
static struct page *ion_page_pool_remove(struct ion_page_pool *pool, bool high)
--
1.7.12.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/2] staging: android: ion: Remove dead code in ion_page_pool_free
ion_page_pool_add will always return 0, however ion_page_pool_free will
call ion_page_pool_free_pages when ion_page_pool_add's return value is
not 0, so it is a dead code which can be removed.
Signed-off-by: Yisheng Xie
---
drivers/staging/android/ion/ion_page_pool.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/staging/android/ion/ion_page_pool.c
b/drivers/staging/android/ion/ion_page_pool.c
index 4452e28..150626f 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -79,13 +79,9 @@ struct page *ion_page_pool_alloc(struct ion_page_pool *pool)
void ion_page_pool_free(struct ion_page_pool *pool, struct page *page)
{
- int ret;
-
BUG_ON(pool->order != compound_order(page));
- ret = ion_page_pool_add(pool, page);
- if (ret)
- ion_page_pool_free_pages(pool, page);
+ ion_page_pool_add(pool, page);
}
static int ion_page_pool_total(struct ion_page_pool *pool, bool high)
--
1.7.12.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
