* [PATCH] staging: iio: adc: Replace mlock with driver private lock
@ 2017-03-12 20:45 Gargi Sharma
2017-03-12 20:56 ` [Outreachy kernel] " Julia Lawall
0 siblings, 1 reply; 22+ messages in thread
From: Gargi Sharma @ 2017-03-12 20:45 UTC (permalink / raw)
To: outreachy-kernel
Cc: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, linux-iio,
Gargi Sharma
The IIO subsystem is redefining iio_dev->mlock to be used by
the IIO core only for protecting device operating mode changes.
ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
In this driver, mlock was being used to protect hardware state
changes. Replace it with a lock in the devices global data.
This was done using Coccinelle Script.
@r1@
identifier xxx_state;
@@
struct xxx_state {
...
+ struct mutex lock; /* protect sensor state */
};
@r2@
expression e;
@@
- mutex_lock(e);
+ mutex_lock(&st->lock);
@r3@
expression e;
@@
- mutex_unlock(e);
+ mutex_unlock(&st->lock);
Signed-off-by: Gargi Sharma <gs051095@gmail.com>
---
drivers/staging/iio/adc/ad7280a.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
index ee679ac..27a3ce3 100644
--- a/drivers/staging/iio/adc/ad7280a.c
+++ b/drivers/staging/iio/adc/ad7280a.c
@@ -136,6 +136,7 @@ struct ad7280_state {
unsigned char cb_mask[AD7280A_MAX_CHAIN];
__be32 buf[2] ____cacheline_aligned;
+ struct mutex lock; /* protect sensor state */
};
static void ad7280_crc8_build_table(unsigned char *crc_tab)
@@ -410,7 +411,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev,
devaddr = this_attr->address >> 8;
ch = this_attr->address & 0xFF;
- mutex_lock(&indio_dev->mlock);
+ mutex_lock(&st->lock);
if (readin)
st->cb_mask[devaddr] |= 1 << (ch + 2);
else
@@ -418,7 +419,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev,
ret = ad7280_write(st, devaddr, AD7280A_CELL_BALANCE,
0, st->cb_mask[devaddr]);
- mutex_unlock(&indio_dev->mlock);
+ mutex_unlock(&st->lock);
return ret ? ret : len;
}
@@ -433,10 +434,10 @@ static ssize_t ad7280_show_balance_timer(struct device *dev,
int ret;
unsigned int msecs;
- mutex_lock(&indio_dev->mlock);
+ mutex_lock(&st->lock);
ret = ad7280_read(st, this_attr->address >> 8,
this_attr->address & 0xFF);
- mutex_unlock(&indio_dev->mlock);
+ mutex_unlock(&st->lock);
if (ret < 0)
return ret;
@@ -466,11 +467,11 @@ static ssize_t ad7280_store_balance_timer(struct device *dev,
if (val > 31)
return -EINVAL;
- mutex_lock(&indio_dev->mlock);
+ mutex_lock(&st->lock);
ret = ad7280_write(st, this_attr->address >> 8,
this_attr->address & 0xFF,
0, (val & 0x1F) << 3);
- mutex_unlock(&indio_dev->mlock);
+ mutex_unlock(&st->lock);
return ret ? ret : len;
}
@@ -655,7 +656,7 @@ static ssize_t ad7280_write_channel_config(struct device *dev,
val = clamp(val, 0L, 0xFFL);
- mutex_lock(&indio_dev->mlock);
+ mutex_lock(&st->lock);
switch ((u32)this_attr->address) {
case AD7280A_CELL_OVERVOLTAGE:
st->cell_threshhigh = val;
@@ -674,7 +675,7 @@ static ssize_t ad7280_write_channel_config(struct device *dev,
ret = ad7280_write(st, AD7280A_DEVADDR_MASTER,
this_attr->address, 1, val);
- mutex_unlock(&indio_dev->mlock);
+ mutex_unlock(&st->lock);
return ret ? ret : len;
}
@@ -792,13 +793,13 @@ static int ad7280_read_raw(struct iio_dev *indio_dev,
switch (m) {
case IIO_CHAN_INFO_RAW:
- mutex_lock(&indio_dev->mlock);
+ mutex_lock(&st->lock);
if (chan->address == AD7280A_ALL_CELLS)
ret = ad7280_read_all_channels(st, st->scan_cnt, NULL);
else
ret = ad7280_read_channel(st, chan->address >> 8,
chan->address & 0xFF);
- mutex_unlock(&indio_dev->mlock);
+ mutex_unlock(&st->lock);
if (ret < 0)
return ret;
--
2.7.4
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock 2017-03-12 20:45 [PATCH] staging: iio: adc: Replace mlock with driver private lock Gargi Sharma @ 2017-03-12 20:56 ` Julia Lawall 2017-03-12 21:33 ` Gargi Sharma 0 siblings, 1 reply; 22+ messages in thread From: Julia Lawall @ 2017-03-12 20:56 UTC (permalink / raw) To: Gargi Sharma Cc: outreachy-kernel, lars, Michael.Hennerich, jic23, knaack.h, pmeerw, linux-iio On Mon, 13 Mar 2017, Gargi Sharma wrote: > The IIO subsystem is redefining iio_dev->mlock to be used by > the IIO core only for protecting device operating mode changes. > ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes. > > In this driver, mlock was being used to protect hardware state > changes. Replace it with a lock in the devices global data. > This was done using Coccinelle Script. > > @r1@ > identifier xxx_state; > @@ > struct xxx_state { > ... > + struct mutex lock; /* protect sensor state */ > }; > > @r2@ > expression e; > @@ > - mutex_lock(e); > + mutex_lock(&st->lock); > > @r3@ > expression e; > @@ > - mutex_unlock(e); > + mutex_unlock(&st->lock); This rule is probably hjelpful in practice, but is very unsafe. Is there any way that you can characterize the kind of structure you want to add a new lock to? And likewise, what lock and unlock calls you want to replace? And what is st? julia > > Signed-off-by: Gargi Sharma <gs051095@gmail.com> > --- > drivers/staging/iio/adc/ad7280a.c | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) > > diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c > index ee679ac..27a3ce3 100644 > --- a/drivers/staging/iio/adc/ad7280a.c > +++ b/drivers/staging/iio/adc/ad7280a.c > @@ -136,6 +136,7 @@ struct ad7280_state { > unsigned char cb_mask[AD7280A_MAX_CHAIN]; > > __be32 buf[2] ____cacheline_aligned; > + struct mutex lock; /* protect sensor state */ > }; > > static void ad7280_crc8_build_table(unsigned char *crc_tab) > @@ -410,7 +411,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev, > devaddr = this_attr->address >> 8; > ch = this_attr->address & 0xFF; > > - mutex_lock(&indio_dev->mlock); > + mutex_lock(&st->lock); > if (readin) > st->cb_mask[devaddr] |= 1 << (ch + 2); > else > @@ -418,7 +419,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev, > > ret = ad7280_write(st, devaddr, AD7280A_CELL_BALANCE, > 0, st->cb_mask[devaddr]); > - mutex_unlock(&indio_dev->mlock); > + mutex_unlock(&st->lock); > > return ret ? ret : len; > } > @@ -433,10 +434,10 @@ static ssize_t ad7280_show_balance_timer(struct device *dev, > int ret; > unsigned int msecs; > > - mutex_lock(&indio_dev->mlock); > + mutex_lock(&st->lock); > ret = ad7280_read(st, this_attr->address >> 8, > this_attr->address & 0xFF); > - mutex_unlock(&indio_dev->mlock); > + mutex_unlock(&st->lock); > > if (ret < 0) > return ret; > @@ -466,11 +467,11 @@ static ssize_t ad7280_store_balance_timer(struct device *dev, > if (val > 31) > return -EINVAL; > > - mutex_lock(&indio_dev->mlock); > + mutex_lock(&st->lock); > ret = ad7280_write(st, this_attr->address >> 8, > this_attr->address & 0xFF, > 0, (val & 0x1F) << 3); > - mutex_unlock(&indio_dev->mlock); > + mutex_unlock(&st->lock); > > return ret ? ret : len; > } > @@ -655,7 +656,7 @@ static ssize_t ad7280_write_channel_config(struct device *dev, > > val = clamp(val, 0L, 0xFFL); > > - mutex_lock(&indio_dev->mlock); > + mutex_lock(&st->lock); > switch ((u32)this_attr->address) { > case AD7280A_CELL_OVERVOLTAGE: > st->cell_threshhigh = val; > @@ -674,7 +675,7 @@ static ssize_t ad7280_write_channel_config(struct device *dev, > ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, > this_attr->address, 1, val); > > - mutex_unlock(&indio_dev->mlock); > + mutex_unlock(&st->lock); > > return ret ? ret : len; > } > @@ -792,13 +793,13 @@ static int ad7280_read_raw(struct iio_dev *indio_dev, > > switch (m) { > case IIO_CHAN_INFO_RAW: > - mutex_lock(&indio_dev->mlock); > + mutex_lock(&st->lock); > if (chan->address == AD7280A_ALL_CELLS) > ret = ad7280_read_all_channels(st, st->scan_cnt, NULL); > else > ret = ad7280_read_channel(st, chan->address >> 8, > chan->address & 0xFF); > - mutex_unlock(&indio_dev->mlock); > + mutex_unlock(&st->lock); > > if (ret < 0) > return ret; > -- > 2.7.4 > > -- > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com. > To post to this group, send email to outreachy-kernel@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1489351546-21008-1-git-send-email-gs051095%40gmail.com. > For more options, visit https://groups.google.com/d/optout. > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock 2017-03-12 20:56 ` [Outreachy kernel] " Julia Lawall @ 2017-03-12 21:33 ` Gargi Sharma 2017-03-12 21:37 ` Julia Lawall 0 siblings, 1 reply; 22+ messages in thread From: Gargi Sharma @ 2017-03-12 21:33 UTC (permalink / raw) To: Julia Lawall Cc: outreachy-kernel, lars, Michael.Hennerich, jic23, knaack.h, Peter Meerwald-Stadler, linux-iio On Mon, Mar 13, 2017 at 2:26 AM, Julia Lawall <julia.lawall@lip6.fr> wrote: > > > On Mon, 13 Mar 2017, Gargi Sharma wrote: > >> The IIO subsystem is redefining iio_dev->mlock to be used by >> the IIO core only for protecting device operating mode changes. >> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes. >> >> In this driver, mlock was being used to protect hardware state >> changes. Replace it with a lock in the devices global data. >> This was done using Coccinelle Script. >> >> @r1@ >> identifier xxx_state; >> @@ >> struct xxx_state { >> ... >> + struct mutex lock; /* protect sensor state */ >> }; >> >> @r2@ >> expression e; >> @@ >> - mutex_lock(e); >> + mutex_lock(&st->lock); >> >> @r3@ >> expression e; >> @@ >> - mutex_unlock(e); >> + mutex_unlock(&st->lock); > > > This rule is probably hjelpful in practice, but is very unsafe. Is there > any way that you can characterize the kind of structure you want to add a > new lock to? And likewise, what lock and unlock calls you want to > replace? And what is st? > Hi! I wanted to do the following things, but was getting a parse error with the Coccinelle Script. st is a structure of type xxx_state. If I inherit xxx_state from r1, to r2 and write the following in the cocci script, for some reason it was not parsing the structure at all. @r2@ expression e1,e2; xxx_state << r1.xxx_state; identifier i; position p; @@ struct xxx_state i@p = e1; //If the script is only till this // point and there is an * in // the beginning, the rule //does not detect a structure. mutex_lock ( - e1 + &i->lock //this gives a parsing error ); > julia > >> >> Signed-off-by: Gargi Sharma <gs051095@gmail.com> >> --- >> drivers/staging/iio/adc/ad7280a.c | 21 +++++++++++---------- >> 1 file changed, 11 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c >> index ee679ac..27a3ce3 100644 >> --- a/drivers/staging/iio/adc/ad7280a.c >> +++ b/drivers/staging/iio/adc/ad7280a.c >> @@ -136,6 +136,7 @@ struct ad7280_state { >> unsigned char cb_mask[AD7280A_MAX_CHAIN]; >> >> __be32 buf[2] ____cacheline_aligned; >> + struct mutex lock; /* protect sensor state */ >> }; >> >> static void ad7280_crc8_build_table(unsigned char *crc_tab) >> @@ -410,7 +411,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev, >> devaddr = this_attr->address >> 8; >> ch = this_attr->address & 0xFF; >> >> - mutex_lock(&indio_dev->mlock); >> + mutex_lock(&st->lock); >> if (readin) >> st->cb_mask[devaddr] |= 1 << (ch + 2); >> else >> @@ -418,7 +419,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev, >> >> ret = ad7280_write(st, devaddr, AD7280A_CELL_BALANCE, >> 0, st->cb_mask[devaddr]); >> - mutex_unlock(&indio_dev->mlock); >> + mutex_unlock(&st->lock); >> >> return ret ? ret : len; >> } >> @@ -433,10 +434,10 @@ static ssize_t ad7280_show_balance_timer(struct device *dev, >> int ret; >> unsigned int msecs; >> >> - mutex_lock(&indio_dev->mlock); >> + mutex_lock(&st->lock); >> ret = ad7280_read(st, this_attr->address >> 8, >> this_attr->address & 0xFF); >> - mutex_unlock(&indio_dev->mlock); >> + mutex_unlock(&st->lock); >> >> if (ret < 0) >> return ret; >> @@ -466,11 +467,11 @@ static ssize_t ad7280_store_balance_timer(struct device *dev, >> if (val > 31) >> return -EINVAL; >> >> - mutex_lock(&indio_dev->mlock); >> + mutex_lock(&st->lock); >> ret = ad7280_write(st, this_attr->address >> 8, >> this_attr->address & 0xFF, >> 0, (val & 0x1F) << 3); >> - mutex_unlock(&indio_dev->mlock); >> + mutex_unlock(&st->lock); >> >> return ret ? ret : len; >> } >> @@ -655,7 +656,7 @@ static ssize_t ad7280_write_channel_config(struct device *dev, >> >> val = clamp(val, 0L, 0xFFL); >> >> - mutex_lock(&indio_dev->mlock); >> + mutex_lock(&st->lock); >> switch ((u32)this_attr->address) { >> case AD7280A_CELL_OVERVOLTAGE: >> st->cell_threshhigh = val; >> @@ -674,7 +675,7 @@ static ssize_t ad7280_write_channel_config(struct device *dev, >> ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, >> this_attr->address, 1, val); >> >> - mutex_unlock(&indio_dev->mlock); >> + mutex_unlock(&st->lock); >> >> return ret ? ret : len; >> } >> @@ -792,13 +793,13 @@ static int ad7280_read_raw(struct iio_dev *indio_dev, >> >> switch (m) { >> case IIO_CHAN_INFO_RAW: >> - mutex_lock(&indio_dev->mlock); >> + mutex_lock(&st->lock); >> if (chan->address == AD7280A_ALL_CELLS) >> ret = ad7280_read_all_channels(st, st->scan_cnt, NULL); >> else >> ret = ad7280_read_channel(st, chan->address >> 8, >> chan->address & 0xFF); >> - mutex_unlock(&indio_dev->mlock); >> + mutex_unlock(&st->lock); >> >> if (ret < 0) >> return ret; >> -- >> 2.7.4 >> >> -- ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock 2017-03-12 21:33 ` Gargi Sharma @ 2017-03-12 21:37 ` Julia Lawall 2017-03-12 22:13 ` Alison Schofield 0 siblings, 1 reply; 22+ messages in thread From: Julia Lawall @ 2017-03-12 21:37 UTC (permalink / raw) To: Gargi Sharma Cc: outreachy-kernel, lars, Michael.Hennerich, jic23, knaack.h, Peter Meerwald-Stadler, linux-iio On Mon, 13 Mar 2017, Gargi Sharma wrote: > On Mon, Mar 13, 2017 at 2:26 AM, Julia Lawall <julia.lawall@lip6.fr> wrote: > > > > > > On Mon, 13 Mar 2017, Gargi Sharma wrote: > > > >> The IIO subsystem is redefining iio_dev->mlock to be used by > >> the IIO core only for protecting device operating mode changes. > >> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes. > >> > >> In this driver, mlock was being used to protect hardware state > >> changes. Replace it with a lock in the devices global data. > >> This was done using Coccinelle Script. > >> > >> @r1@ > >> identifier xxx_state; > >> @@ > >> struct xxx_state { > >> ... > >> + struct mutex lock; /* protect sensor state */ > >> }; > >> > >> @r2@ > >> expression e; > >> @@ > >> - mutex_lock(e); > >> + mutex_lock(&st->lock); > >> > >> @r3@ > >> expression e; > >> @@ > >> - mutex_unlock(e); > >> + mutex_unlock(&st->lock); > > > > > > This rule is probably hjelpful in practice, but is very unsafe. Is there > > any way that you can characterize the kind of structure you want to add a > > new lock to? And likewise, what lock and unlock calls you want to > > replace? And what is st? > > > Hi! > > I wanted to do the following things, but was getting a parse error with the > Coccinelle Script. > > st is a structure of type xxx_state. If I inherit xxx_state from r1, > to r2 and write > the following in the cocci script, for some reason it was not parsing > the structure at all. > > @r2@ > expression e1,e2; > xxx_state << r1.xxx_state; << is the way you inherit a metavariable in script code. The first script language available was python and it was considered to be odd to ask people to put types on variables to be used in python code. In any case, all kinds of terms are represented as strings, so the tpye would not be very useful. When you are in pattern matching (SmPL) code, metavariables are inherited as identifier r1.xxx_state; (or whatever is the appropriate type) julia > identifier i; > position p; > @@ > struct xxx_state i@p = e1; //If the script is only till this > // point and there is an * in > // the beginning, the rule > //does not detect a structure. > mutex_lock > ( > - e1 > + &i->lock //this gives a parsing error > ); > > > > julia > > > >> > >> Signed-off-by: Gargi Sharma <gs051095@gmail.com> > >> --- > >> drivers/staging/iio/adc/ad7280a.c | 21 +++++++++++---------- > >> 1 file changed, 11 insertions(+), 10 deletions(-) > >> > >> diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c > >> index ee679ac..27a3ce3 100644 > >> --- a/drivers/staging/iio/adc/ad7280a.c > >> +++ b/drivers/staging/iio/adc/ad7280a.c > >> @@ -136,6 +136,7 @@ struct ad7280_state { > >> unsigned char cb_mask[AD7280A_MAX_CHAIN]; > >> > >> __be32 buf[2] ____cacheline_aligned; > >> + struct mutex lock; /* protect sensor state */ > >> }; > >> > >> static void ad7280_crc8_build_table(unsigned char *crc_tab) > >> @@ -410,7 +411,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev, > >> devaddr = this_attr->address >> 8; > >> ch = this_attr->address & 0xFF; > >> > >> - mutex_lock(&indio_dev->mlock); > >> + mutex_lock(&st->lock); > >> if (readin) > >> st->cb_mask[devaddr] |= 1 << (ch + 2); > >> else > >> @@ -418,7 +419,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev, > >> > >> ret = ad7280_write(st, devaddr, AD7280A_CELL_BALANCE, > >> 0, st->cb_mask[devaddr]); > >> - mutex_unlock(&indio_dev->mlock); > >> + mutex_unlock(&st->lock); > >> > >> return ret ? ret : len; > >> } > >> @@ -433,10 +434,10 @@ static ssize_t ad7280_show_balance_timer(struct device *dev, > >> int ret; > >> unsigned int msecs; > >> > >> - mutex_lock(&indio_dev->mlock); > >> + mutex_lock(&st->lock); > >> ret = ad7280_read(st, this_attr->address >> 8, > >> this_attr->address & 0xFF); > >> - mutex_unlock(&indio_dev->mlock); > >> + mutex_unlock(&st->lock); > >> > >> if (ret < 0) > >> return ret; > >> @@ -466,11 +467,11 @@ static ssize_t ad7280_store_balance_timer(struct device *dev, > >> if (val > 31) > >> return -EINVAL; > >> > >> - mutex_lock(&indio_dev->mlock); > >> + mutex_lock(&st->lock); > >> ret = ad7280_write(st, this_attr->address >> 8, > >> this_attr->address & 0xFF, > >> 0, (val & 0x1F) << 3); > >> - mutex_unlock(&indio_dev->mlock); > >> + mutex_unlock(&st->lock); > >> > >> return ret ? ret : len; > >> } > >> @@ -655,7 +656,7 @@ static ssize_t ad7280_write_channel_config(struct device *dev, > >> > >> val = clamp(val, 0L, 0xFFL); > >> > >> - mutex_lock(&indio_dev->mlock); > >> + mutex_lock(&st->lock); > >> switch ((u32)this_attr->address) { > >> case AD7280A_CELL_OVERVOLTAGE: > >> st->cell_threshhigh = val; > >> @@ -674,7 +675,7 @@ static ssize_t ad7280_write_channel_config(struct device *dev, > >> ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, > >> this_attr->address, 1, val); > >> > >> - mutex_unlock(&indio_dev->mlock); > >> + mutex_unlock(&st->lock); > >> > >> return ret ? ret : len; > >> } > >> @@ -792,13 +793,13 @@ static int ad7280_read_raw(struct iio_dev *indio_dev, > >> > >> switch (m) { > >> case IIO_CHAN_INFO_RAW: > >> - mutex_lock(&indio_dev->mlock); > >> + mutex_lock(&st->lock); > >> if (chan->address == AD7280A_ALL_CELLS) > >> ret = ad7280_read_all_channels(st, st->scan_cnt, NULL); > >> else > >> ret = ad7280_read_channel(st, chan->address >> 8, > >> chan->address & 0xFF); > >> - mutex_unlock(&indio_dev->mlock); > >> + mutex_unlock(&st->lock); > >> > >> if (ret < 0) > >> return ret; > >> -- > >> 2.7.4 > >> > >> -- > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock 2017-03-12 21:37 ` Julia Lawall @ 2017-03-12 22:13 ` Alison Schofield 2017-03-12 22:27 ` Julia Lawall 2017-03-13 8:40 ` Gargi Sharma 0 siblings, 2 replies; 22+ messages in thread From: Alison Schofield @ 2017-03-12 22:13 UTC (permalink / raw) To: Julia Lawall Cc: Gargi Sharma, outreachy-kernel, lars, Michael.Hennerich, jic23, knaack.h, Peter Meerwald-Stadler, linux-iio On Sun, Mar 12, 2017 at 10:37:39PM +0100, Julia Lawall wrote: > > > On Mon, 13 Mar 2017, Gargi Sharma wrote: > > > On Mon, Mar 13, 2017 at 2:26 AM, Julia Lawall <julia.lawall@lip6.fr> wrote: > > > > > > > > > On Mon, 13 Mar 2017, Gargi Sharma wrote: > > > > > >> The IIO subsystem is redefining iio_dev->mlock to be used by > > >> the IIO core only for protecting device operating mode changes. > > >> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes. > > >> > > >> In this driver, mlock was being used to protect hardware state > > >> changes. Replace it with a lock in the devices global data. > > >> This was done using Coccinelle Script. > > >> > > >> @r1@ > > >> identifier xxx_state; > > >> @@ > > >> struct xxx_state { > > >> ... > > >> + struct mutex lock; /* protect sensor state */ > > >> }; > > >> > > >> @r2@ > > >> expression e; > > >> @@ > > >> - mutex_lock(e); > > >> + mutex_lock(&st->lock); > > >> > > >> @r3@ > > >> expression e; > > >> @@ > > >> - mutex_unlock(e); > > >> + mutex_unlock(&st->lock); > > > > > > > > > This rule is probably hjelpful in practice, but is very unsafe. Is there > > > any way that you can characterize the kind of structure you want to add a > > > new lock to? And likewise, what lock and unlock calls you want to > > > replace? And what is st? > > > > > Hi! > > > > I wanted to do the following things, but was getting a parse error with the > > Coccinelle Script. > > > > st is a structure of type xxx_state. If I inherit xxx_state from r1, > > to r2 and write > > the following in the cocci script, for some reason it was not parsing > > the structure at all. > > > > @r2@ > > expression e1,e2; > > xxx_state << r1.xxx_state; > > << is the way you inherit a metavariable in script code. The first script > language available was python and it was considered to be odd to ask > people to put types on variables to be used in python code. In any case, > all kinds of terms are represented as strings, so the tpye would not be > very useful. > > When you are in pattern matching (SmPL) code, metavariables are inherited > as > > identifier r1.xxx_state; > > (or whatever is the appropriate type) > > julia > > > identifier i; > > position p; > > @@ > > struct xxx_state i@p = e1; //If the script is only till this > > // point and there is an * in > > // the beginning, the rule > > //does not detect a structure. > > mutex_lock > > ( > > - e1 > > + &i->lock //this gives a parsing error > > ); > > > > > > > julia > > > Gargi, Please insert the new lock above the __cacheline_aligned struct member. I like the automated editing because it certainly cuts down on typos, and even with this one, you can just move the new lock field upon inspection. Sadly, I don't think the script will apply too widely because the use of "_state" for driver global data isn't a standard, nor availability of that struct in the functions needing the locking. That sadness expressed ;) we do have a boatload of drivers upstream that will eventually make this migration, so the value of the cocci script would go beyond these 15 staging drivers. That's valuable. Let's make sure anything done with any automated tools is walked through thoroughly. Per the task description: this is not intended as a search&replace exercise. alisons > > >> > > >> Signed-off-by: Gargi Sharma <gs051095@gmail.com> > > >> --- > > >> drivers/staging/iio/adc/ad7280a.c | 21 +++++++++++---------- > > >> 1 file changed, 11 insertions(+), 10 deletions(-) > > >> > > >> diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c > > >> index ee679ac..27a3ce3 100644 > > >> --- a/drivers/staging/iio/adc/ad7280a.c > > >> +++ b/drivers/staging/iio/adc/ad7280a.c > > >> @@ -136,6 +136,7 @@ struct ad7280_state { > > >> unsigned char cb_mask[AD7280A_MAX_CHAIN]; > > >> > > >> __be32 buf[2] ____cacheline_aligned; > > >> + struct mutex lock; /* protect sensor state */ > > >> }; > > >> > > >> static void ad7280_crc8_build_table(unsigned char *crc_tab) > > >> @@ -410,7 +411,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev, > > >> devaddr = this_attr->address >> 8; > > >> ch = this_attr->address & 0xFF; > > >> > > >> - mutex_lock(&indio_dev->mlock); > > >> + mutex_lock(&st->lock); > > >> if (readin) > > >> st->cb_mask[devaddr] |= 1 << (ch + 2); > > >> else > > >> @@ -418,7 +419,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev, > > >> > > >> ret = ad7280_write(st, devaddr, AD7280A_CELL_BALANCE, > > >> 0, st->cb_mask[devaddr]); > > >> - mutex_unlock(&indio_dev->mlock); > > >> + mutex_unlock(&st->lock); > > >> > > >> return ret ? ret : len; > > >> } > > >> @@ -433,10 +434,10 @@ static ssize_t ad7280_show_balance_timer(struct device *dev, > > >> int ret; > > >> unsigned int msecs; > > >> > > >> - mutex_lock(&indio_dev->mlock); > > >> + mutex_lock(&st->lock); > > >> ret = ad7280_read(st, this_attr->address >> 8, > > >> this_attr->address & 0xFF); > > >> - mutex_unlock(&indio_dev->mlock); > > >> + mutex_unlock(&st->lock); > > >> > > >> if (ret < 0) > > >> return ret; > > >> @@ -466,11 +467,11 @@ static ssize_t ad7280_store_balance_timer(struct device *dev, > > >> if (val > 31) > > >> return -EINVAL; > > >> > > >> - mutex_lock(&indio_dev->mlock); > > >> + mutex_lock(&st->lock); > > >> ret = ad7280_write(st, this_attr->address >> 8, > > >> this_attr->address & 0xFF, > > >> 0, (val & 0x1F) << 3); > > >> - mutex_unlock(&indio_dev->mlock); > > >> + mutex_unlock(&st->lock); > > >> > > >> return ret ? ret : len; > > >> } > > >> @@ -655,7 +656,7 @@ static ssize_t ad7280_write_channel_config(struct device *dev, > > >> > > >> val = clamp(val, 0L, 0xFFL); > > >> > > >> - mutex_lock(&indio_dev->mlock); > > >> + mutex_lock(&st->lock); > > >> switch ((u32)this_attr->address) { > > >> case AD7280A_CELL_OVERVOLTAGE: > > >> st->cell_threshhigh = val; > > >> @@ -674,7 +675,7 @@ static ssize_t ad7280_write_channel_config(struct device *dev, > > >> ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, > > >> this_attr->address, 1, val); > > >> > > >> - mutex_unlock(&indio_dev->mlock); > > >> + mutex_unlock(&st->lock); > > >> > > >> return ret ? ret : len; > > >> } > > >> @@ -792,13 +793,13 @@ static int ad7280_read_raw(struct iio_dev *indio_dev, > > >> > > >> switch (m) { > > >> case IIO_CHAN_INFO_RAW: > > >> - mutex_lock(&indio_dev->mlock); > > >> + mutex_lock(&st->lock); > > >> if (chan->address == AD7280A_ALL_CELLS) > > >> ret = ad7280_read_all_channels(st, st->scan_cnt, NULL); > > >> else > > >> ret = ad7280_read_channel(st, chan->address >> 8, > > >> chan->address & 0xFF); > > >> - mutex_unlock(&indio_dev->mlock); > > >> + mutex_unlock(&st->lock); > > >> > > >> if (ret < 0) > > >> return ret; > > >> -- > > >> 2.7.4 > > >> > > >> -- > > > > -- > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com. > To post to this group, send email to outreachy-kernel@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/alpine.DEB.2.20.1703122235320.7649%40hadrien. > For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock 2017-03-12 22:13 ` Alison Schofield @ 2017-03-12 22:27 ` Julia Lawall 2017-03-13 8:41 ` Gargi Sharma 2017-03-13 8:40 ` Gargi Sharma 1 sibling, 1 reply; 22+ messages in thread From: Julia Lawall @ 2017-03-12 22:27 UTC (permalink / raw) To: Alison Schofield Cc: Gargi Sharma, outreachy-kernel, lars, Michael.Hennerich, jic23, knaack.h, Peter Meerwald-Stadler, linux-iio > Please insert the new lock above the __cacheline_aligned struct > member. Regrettably, at the moment, Coccinelle can't help you find this. It could help if there is something particular about the type. > I like the automated editing because it certainly cuts down on typos, > and even with this one, you can just move the new lock field upon > inspection. Sadly, I don't think the script will apply too widely > because the use of "_state" for driver global data isn't a standard, > nor availability of that struct in the functions needing the locking. The _state is part of a metavariable name. It doesn't have any impact on what is matched. > That sadness expressed ;) we do have a boatload of drivers upstream > that will eventually make this migration, so the value of the cocci > script would go beyond these 15 staging drivers. That's valuable. > > Let's make sure anything done with any automated tools is walked > through thoroughly. Per the task description: this is not intended > as a search&replace exercise. Automation is great, modulo the need to manually check the result. However, it could be better not to include in the commit log semantic patches that are too far off from what updates the driver in a safe way. It doesn't have to update all drivers safely, but it should work for the modified one. The goal should be to complement the log message text, not to make the reader confused :) julia > alisons > > > > > >> > > > >> Signed-off-by: Gargi Sharma <gs051095@gmail.com> > > > >> --- > > > >> drivers/staging/iio/adc/ad7280a.c | 21 +++++++++++---------- > > > >> 1 file changed, 11 insertions(+), 10 deletions(-) > > > >> > > > >> diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c > > > >> index ee679ac..27a3ce3 100644 > > > >> --- a/drivers/staging/iio/adc/ad7280a.c > > > >> +++ b/drivers/staging/iio/adc/ad7280a.c > > > >> @@ -136,6 +136,7 @@ struct ad7280_state { > > > >> unsigned char cb_mask[AD7280A_MAX_CHAIN]; > > > >> > > > >> __be32 buf[2] ____cacheline_aligned; > > > >> + struct mutex lock; /* protect sensor state */ > > > >> }; > > > >> > > > >> static void ad7280_crc8_build_table(unsigned char *crc_tab) > > > >> @@ -410,7 +411,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev, > > > >> devaddr = this_attr->address >> 8; > > > >> ch = this_attr->address & 0xFF; > > > >> > > > >> - mutex_lock(&indio_dev->mlock); > > > >> + mutex_lock(&st->lock); > > > >> if (readin) > > > >> st->cb_mask[devaddr] |= 1 << (ch + 2); > > > >> else > > > >> @@ -418,7 +419,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev, > > > >> > > > >> ret = ad7280_write(st, devaddr, AD7280A_CELL_BALANCE, > > > >> 0, st->cb_mask[devaddr]); > > > >> - mutex_unlock(&indio_dev->mlock); > > > >> + mutex_unlock(&st->lock); > > > >> > > > >> return ret ? ret : len; > > > >> } > > > >> @@ -433,10 +434,10 @@ static ssize_t ad7280_show_balance_timer(struct device *dev, > > > >> int ret; > > > >> unsigned int msecs; > > > >> > > > >> - mutex_lock(&indio_dev->mlock); > > > >> + mutex_lock(&st->lock); > > > >> ret = ad7280_read(st, this_attr->address >> 8, > > > >> this_attr->address & 0xFF); > > > >> - mutex_unlock(&indio_dev->mlock); > > > >> + mutex_unlock(&st->lock); > > > >> > > > >> if (ret < 0) > > > >> return ret; > > > >> @@ -466,11 +467,11 @@ static ssize_t ad7280_store_balance_timer(struct device *dev, > > > >> if (val > 31) > > > >> return -EINVAL; > > > >> > > > >> - mutex_lock(&indio_dev->mlock); > > > >> + mutex_lock(&st->lock); > > > >> ret = ad7280_write(st, this_attr->address >> 8, > > > >> this_attr->address & 0xFF, > > > >> 0, (val & 0x1F) << 3); > > > >> - mutex_unlock(&indio_dev->mlock); > > > >> + mutex_unlock(&st->lock); > > > >> > > > >> return ret ? ret : len; > > > >> } > > > >> @@ -655,7 +656,7 @@ static ssize_t ad7280_write_channel_config(struct device *dev, > > > >> > > > >> val = clamp(val, 0L, 0xFFL); > > > >> > > > >> - mutex_lock(&indio_dev->mlock); > > > >> + mutex_lock(&st->lock); > > > >> switch ((u32)this_attr->address) { > > > >> case AD7280A_CELL_OVERVOLTAGE: > > > >> st->cell_threshhigh = val; > > > >> @@ -674,7 +675,7 @@ static ssize_t ad7280_write_channel_config(struct device *dev, > > > >> ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, > > > >> this_attr->address, 1, val); > > > >> > > > >> - mutex_unlock(&indio_dev->mlock); > > > >> + mutex_unlock(&st->lock); > > > >> > > > >> return ret ? ret : len; > > > >> } > > > >> @@ -792,13 +793,13 @@ static int ad7280_read_raw(struct iio_dev *indio_dev, > > > >> > > > >> switch (m) { > > > >> case IIO_CHAN_INFO_RAW: > > > >> - mutex_lock(&indio_dev->mlock); > > > >> + mutex_lock(&st->lock); > > > >> if (chan->address == AD7280A_ALL_CELLS) > > > >> ret = ad7280_read_all_channels(st, st->scan_cnt, NULL); > > > >> else > > > >> ret = ad7280_read_channel(st, chan->address >> 8, > > > >> chan->address & 0xFF); > > > >> - mutex_unlock(&indio_dev->mlock); > > > >> + mutex_unlock(&st->lock); > > > >> > > > >> if (ret < 0) > > > >> return ret; > > > >> -- > > > >> 2.7.4 > > > >> > > > >> -- > > > > > > > -- > > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. > > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com. > > To post to this group, send email to outreachy-kernel@googlegroups.com. > > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/alpine.DEB.2.20.1703122235320.7649%40hadrien. > > For more options, visit https://groups.google.com/d/optout. > > -- > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com. > To post to this group, send email to outreachy-kernel@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170312221350.GA14070%40d830.WORKGROUP. > For more options, visit https://groups.google.com/d/optout. > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock 2017-03-12 22:27 ` Julia Lawall @ 2017-03-13 8:41 ` Gargi Sharma 2017-03-15 9:44 ` Julia Lawall 0 siblings, 1 reply; 22+ messages in thread From: Gargi Sharma @ 2017-03-13 8:41 UTC (permalink / raw) To: Julia Lawall Cc: Alison Schofield, outreachy-kernel, lars, Michael.Hennerich, jic23, knaack.h, Peter Meerwald-Stadler, linux-iio On Mon, Mar 13, 2017 at 3:57 AM, Julia Lawall <julia.lawall@lip6.fr> wrote: >> Please insert the new lock above the __cacheline_aligned struct >> member. > > Regrettably, at the moment, Coccinelle can't help you find this. It could > help if there is something particular about the type. > > >> I like the automated editing because it certainly cuts down on typos, >> and even with this one, you can just move the new lock field upon >> inspection. Sadly, I don't think the script will apply too widely >> because the use of "_state" for driver global data isn't a standard, >> nor availability of that struct in the functions needing the locking. > > The _state is part of a metavariable name. It doesn't have any impact on > what is matched. > >> That sadness expressed ;) we do have a boatload of drivers upstream >> that will eventually make this migration, so the value of the cocci >> script would go beyond these 15 staging drivers. That's valuable. >> >> Let's make sure anything done with any automated tools is walked >> through thoroughly. Per the task description: this is not intended >> as a search&replace exercise. > > Automation is great, modulo the need to manually check the result. > However, it could be better not to include in the commit log semantic > patches that are too far off from what updates the driver in a safe way. > It doesn't have to update all drivers safely, but it should work for the > modified one. The goal should be to complement the log message text, not > to make the reader confused :) Will it be better if I remove the script from the commit log here? That way the confusion can be prevented :) Thanks! Gargi > > julia > >> alisons >> >> >> > > >> >> > > >> Signed-off-by: Gargi Sharma <gs051095@gmail.com> >> > > >> --- >> > > >> drivers/staging/iio/adc/ad7280a.c | 21 +++++++++++---------- >> > > >> 1 file changed, 11 insertions(+), 10 deletions(-) >> > > >> >> > > >> diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c >> > > >> index ee679ac..27a3ce3 100644 >> > > >> --- a/drivers/staging/iio/adc/ad7280a.c >> > > >> +++ b/drivers/staging/iio/adc/ad7280a.c >> > > >> @@ -136,6 +136,7 @@ struct ad7280_state { >> > > >> unsigned char cb_mask[AD7280A_MAX_CHAIN]; >> > > >> >> > > >> __be32 buf[2] ____cacheline_aligned; >> > > >> + struct mutex lock; /* protect sensor state */ >> > > >> }; >> > > >> >> > > >> static void ad7280_crc8_build_table(unsigned char *crc_tab) >> > > >> @@ -410,7 +411,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev, >> > > >> devaddr = this_attr->address >> 8; >> > > >> ch = this_attr->address & 0xFF; >> > > >> >> > > >> - mutex_lock(&indio_dev->mlock); >> > > >> + mutex_lock(&st->lock); >> > > >> if (readin) >> > > >> st->cb_mask[devaddr] |= 1 << (ch + 2); >> > > >> else >> > > >> @@ -418,7 +419,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev, >> > > >> >> > > >> ret = ad7280_write(st, devaddr, AD7280A_CELL_BALANCE, >> > > >> 0, st->cb_mask[devaddr]); >> > > >> - mutex_unlock(&indio_dev->mlock); >> > > >> + mutex_unlock(&st->lock); >> > > >> >> > > >> return ret ? ret : len; >> > > >> } >> > > >> @@ -433,10 +434,10 @@ static ssize_t ad7280_show_balance_timer(struct device *dev, >> > > >> int ret; >> > > >> unsigned int msecs; >> > > >> >> > > >> - mutex_lock(&indio_dev->mlock); >> > > >> + mutex_lock(&st->lock); >> > > >> ret = ad7280_read(st, this_attr->address >> 8, >> > > >> this_attr->address & 0xFF); >> > > >> - mutex_unlock(&indio_dev->mlock); >> > > >> + mutex_unlock(&st->lock); >> > > >> >> > > >> if (ret < 0) >> > > >> return ret; >> > > >> @@ -466,11 +467,11 @@ static ssize_t ad7280_store_balance_timer(struct device *dev, >> > > >> if (val > 31) >> > > >> return -EINVAL; >> > > >> >> > > >> - mutex_lock(&indio_dev->mlock); >> > > >> + mutex_lock(&st->lock); >> > > >> ret = ad7280_write(st, this_attr->address >> 8, >> > > >> this_attr->address & 0xFF, >> > > >> 0, (val & 0x1F) << 3); >> > > >> - mutex_unlock(&indio_dev->mlock); >> > > >> + mutex_unlock(&st->lock); >> > > >> >> > > >> return ret ? ret : len; >> > > >> } >> > > >> @@ -655,7 +656,7 @@ static ssize_t ad7280_write_channel_config(struct device *dev, >> > > >> >> > > >> val = clamp(val, 0L, 0xFFL); >> > > >> >> > > >> - mutex_lock(&indio_dev->mlock); >> > > >> + mutex_lock(&st->lock); >> > > >> switch ((u32)this_attr->address) { >> > > >> case AD7280A_CELL_OVERVOLTAGE: >> > > >> st->cell_threshhigh = val; >> > > >> @@ -674,7 +675,7 @@ static ssize_t ad7280_write_channel_config(struct device *dev, >> > > >> ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, >> > > >> this_attr->address, 1, val); >> > > >> >> > > >> - mutex_unlock(&indio_dev->mlock); >> > > >> + mutex_unlock(&st->lock); >> > > >> >> > > >> return ret ? ret : len; >> > > >> } >> > > >> @@ -792,13 +793,13 @@ static int ad7280_read_raw(struct iio_dev *indio_dev, >> > > >> >> > > >> switch (m) { >> > > >> case IIO_CHAN_INFO_RAW: >> > > >> - mutex_lock(&indio_dev->mlock); >> > > >> + mutex_lock(&st->lock); >> > > >> if (chan->address == AD7280A_ALL_CELLS) >> > > >> ret = ad7280_read_all_channels(st, st->scan_cnt, NULL); >> > > >> else >> > > >> ret = ad7280_read_channel(st, chan->address >> 8, >> > > >> chan->address & 0xFF); >> > > >> - mutex_unlock(&indio_dev->mlock); >> > > >> + mutex_unlock(&st->lock); >> > > >> >> > > >> if (ret < 0) >> > > >> return ret; >> > > >> -- >> > > >> 2.7.4 >> > > >> >> > > >> -- >> > > >> > >> > -- ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock 2017-03-13 8:41 ` Gargi Sharma @ 2017-03-15 9:44 ` Julia Lawall 0 siblings, 0 replies; 22+ messages in thread From: Julia Lawall @ 2017-03-15 9:44 UTC (permalink / raw) To: Gargi Sharma Cc: Julia Lawall, Alison Schofield, outreachy-kernel, lars, Michael.Hennerich, jic23, knaack.h, Peter Meerwald-Stadler, linux-iio On Mon, 13 Mar 2017, Gargi Sharma wrote: > On Mon, Mar 13, 2017 at 3:57 AM, Julia Lawall <julia.lawall@lip6.fr> wrote: > >> Please insert the new lock above the __cacheline_aligned struct > >> member. > > > > Regrettably, at the moment, Coccinelle can't help you find this. It could > > help if there is something particular about the type. > > > > > >> I like the automated editing because it certainly cuts down on typos, > >> and even with this one, you can just move the new lock field upon > >> inspection. Sadly, I don't think the script will apply too widely > >> because the use of "_state" for driver global data isn't a standard, > >> nor availability of that struct in the functions needing the locking. > > > > The _state is part of a metavariable name. It doesn't have any impact on > > what is matched. > > > >> That sadness expressed ;) we do have a boatload of drivers upstream > >> that will eventually make this migration, so the value of the cocci > >> script would go beyond these 15 staging drivers. That's valuable. > >> > >> Let's make sure anything done with any automated tools is walked > >> through thoroughly. Per the task description: this is not intended > >> as a search&replace exercise. > > > > Automation is great, modulo the need to manually check the result. > > However, it could be better not to include in the commit log semantic > > patches that are too far off from what updates the driver in a safe way. > > It doesn't have to update all drivers safely, but it should work for the > > modified one. The goal should be to complement the log message text, not > > to make the reader confused :) > > Will it be better if I remove the script from the commit log here? That way the > confusion can be prevented :) Perhaps a good idea. julia > > Thanks! > Gargi > > > > julia > > > >> alisons > >> > >> > >> > > >> > >> > > >> Signed-off-by: Gargi Sharma <gs051095@gmail.com> > >> > > >> --- > >> > > >> drivers/staging/iio/adc/ad7280a.c | 21 +++++++++++---------- > >> > > >> 1 file changed, 11 insertions(+), 10 deletions(-) > >> > > >> > >> > > >> diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c > >> > > >> index ee679ac..27a3ce3 100644 > >> > > >> --- a/drivers/staging/iio/adc/ad7280a.c > >> > > >> +++ b/drivers/staging/iio/adc/ad7280a.c > >> > > >> @@ -136,6 +136,7 @@ struct ad7280_state { > >> > > >> unsigned char cb_mask[AD7280A_MAX_CHAIN]; > >> > > >> > >> > > >> __be32 buf[2] ____cacheline_aligned; > >> > > >> + struct mutex lock; /* protect sensor state */ > >> > > >> }; > >> > > >> > >> > > >> static void ad7280_crc8_build_table(unsigned char *crc_tab) > >> > > >> @@ -410,7 +411,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev, > >> > > >> devaddr = this_attr->address >> 8; > >> > > >> ch = this_attr->address & 0xFF; > >> > > >> > >> > > >> - mutex_lock(&indio_dev->mlock); > >> > > >> + mutex_lock(&st->lock); > >> > > >> if (readin) > >> > > >> st->cb_mask[devaddr] |= 1 << (ch + 2); > >> > > >> else > >> > > >> @@ -418,7 +419,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev, > >> > > >> > >> > > >> ret = ad7280_write(st, devaddr, AD7280A_CELL_BALANCE, > >> > > >> 0, st->cb_mask[devaddr]); > >> > > >> - mutex_unlock(&indio_dev->mlock); > >> > > >> + mutex_unlock(&st->lock); > >> > > >> > >> > > >> return ret ? ret : len; > >> > > >> } > >> > > >> @@ -433,10 +434,10 @@ static ssize_t ad7280_show_balance_timer(struct device *dev, > >> > > >> int ret; > >> > > >> unsigned int msecs; > >> > > >> > >> > > >> - mutex_lock(&indio_dev->mlock); > >> > > >> + mutex_lock(&st->lock); > >> > > >> ret = ad7280_read(st, this_attr->address >> 8, > >> > > >> this_attr->address & 0xFF); > >> > > >> - mutex_unlock(&indio_dev->mlock); > >> > > >> + mutex_unlock(&st->lock); > >> > > >> > >> > > >> if (ret < 0) > >> > > >> return ret; > >> > > >> @@ -466,11 +467,11 @@ static ssize_t ad7280_store_balance_timer(struct device *dev, > >> > > >> if (val > 31) > >> > > >> return -EINVAL; > >> > > >> > >> > > >> - mutex_lock(&indio_dev->mlock); > >> > > >> + mutex_lock(&st->lock); > >> > > >> ret = ad7280_write(st, this_attr->address >> 8, > >> > > >> this_attr->address & 0xFF, > >> > > >> 0, (val & 0x1F) << 3); > >> > > >> - mutex_unlock(&indio_dev->mlock); > >> > > >> + mutex_unlock(&st->lock); > >> > > >> > >> > > >> return ret ? ret : len; > >> > > >> } > >> > > >> @@ -655,7 +656,7 @@ static ssize_t ad7280_write_channel_config(struct device *dev, > >> > > >> > >> > > >> val = clamp(val, 0L, 0xFFL); > >> > > >> > >> > > >> - mutex_lock(&indio_dev->mlock); > >> > > >> + mutex_lock(&st->lock); > >> > > >> switch ((u32)this_attr->address) { > >> > > >> case AD7280A_CELL_OVERVOLTAGE: > >> > > >> st->cell_threshhigh = val; > >> > > >> @@ -674,7 +675,7 @@ static ssize_t ad7280_write_channel_config(struct device *dev, > >> > > >> ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, > >> > > >> this_attr->address, 1, val); > >> > > >> > >> > > >> - mutex_unlock(&indio_dev->mlock); > >> > > >> + mutex_unlock(&st->lock); > >> > > >> > >> > > >> return ret ? ret : len; > >> > > >> } > >> > > >> @@ -792,13 +793,13 @@ static int ad7280_read_raw(struct iio_dev *indio_dev, > >> > > >> > >> > > >> switch (m) { > >> > > >> case IIO_CHAN_INFO_RAW: > >> > > >> - mutex_lock(&indio_dev->mlock); > >> > > >> + mutex_lock(&st->lock); > >> > > >> if (chan->address == AD7280A_ALL_CELLS) > >> > > >> ret = ad7280_read_all_channels(st, st->scan_cnt, NULL); > >> > > >> else > >> > > >> ret = ad7280_read_channel(st, chan->address >> 8, > >> > > >> chan->address & 0xFF); > >> > > >> - mutex_unlock(&indio_dev->mlock); > >> > > >> + mutex_unlock(&st->lock); > >> > > >> > >> > > >> if (ret < 0) > >> > > >> return ret; > >> > > >> -- > >> > > >> 2.7.4 > >> > > >> > >> > > >> -- > >> > > > >> > > >> > -- > > -- > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com. > To post to this group, send email to outreachy-kernel@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/CAOCi2DGcpoN791cH82nKWYdYdS82MmfJeUpcTcUSG9cbEw3Q4g%40mail.gmail.com. > For more options, visit https://groups.google.com/d/optout. > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock 2017-03-12 22:13 ` Alison Schofield 2017-03-12 22:27 ` Julia Lawall @ 2017-03-13 8:40 ` Gargi Sharma 2017-03-13 20:40 ` Julia Lawall 2017-03-13 20:51 ` Jonathan Cameron 1 sibling, 2 replies; 22+ messages in thread From: Gargi Sharma @ 2017-03-13 8:40 UTC (permalink / raw) To: Alison Schofield Cc: Julia Lawall, outreachy-kernel, lars, Michael.Hennerich, jic23, knaack.h, Peter Meerwald-Stadler, linux-iio On Mon, Mar 13, 2017 at 3:43 AM, Alison Schofield <amsfield22@gmail.com> wrote: > On Sun, Mar 12, 2017 at 10:37:39PM +0100, Julia Lawall wrote: >> >> >> On Mon, 13 Mar 2017, Gargi Sharma wrote: >> >> > On Mon, Mar 13, 2017 at 2:26 AM, Julia Lawall <julia.lawall@lip6.fr> wrote: >> > > >> > > >> > > On Mon, 13 Mar 2017, Gargi Sharma wrote: >> > > >> > >> The IIO subsystem is redefining iio_dev->mlock to be used by >> > >> the IIO core only for protecting device operating mode changes. >> > >> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes. >> > >> >> > >> In this driver, mlock was being used to protect hardware state >> > >> changes. Replace it with a lock in the devices global data. >> > >> This was done using Coccinelle Script. >> > >> >> > >> @r1@ >> > >> identifier xxx_state; >> > >> @@ >> > >> struct xxx_state { >> > >> ... >> > >> + struct mutex lock; /* protect sensor state */ >> > >> }; >> > >> >> > >> @r2@ >> > >> expression e; >> > >> @@ >> > >> - mutex_lock(e); >> > >> + mutex_lock(&st->lock); >> > >> >> > >> @r3@ >> > >> expression e; >> > >> @@ >> > >> - mutex_unlock(e); >> > >> + mutex_unlock(&st->lock); >> > > >> > > >> > > This rule is probably hjelpful in practice, but is very unsafe. Is there >> > > any way that you can characterize the kind of structure you want to add a >> > > new lock to? And likewise, what lock and unlock calls you want to >> > > replace? And what is st? >> > > >> > Hi! >> > >> > I wanted to do the following things, but was getting a parse error with the >> > Coccinelle Script. >> > >> > st is a structure of type xxx_state. If I inherit xxx_state from r1, >> > to r2 and write >> > the following in the cocci script, for some reason it was not parsing >> > the structure at all. >> > >> > @r2@ >> > expression e1,e2; >> > xxx_state << r1.xxx_state; >> >> << is the way you inherit a metavariable in script code. The first script >> language available was python and it was considered to be odd to ask >> people to put types on variables to be used in python code. In any case, >> all kinds of terms are represented as strings, so the tpye would not be >> very useful. >> >> When you are in pattern matching (SmPL) code, metavariables are inherited >> as >> >> identifier r1.xxx_state; >> >> (or whatever is the appropriate type) >> >> julia >> >> > identifier i; >> > position p; >> > @@ >> > struct xxx_state i@p = e1; //If the script is only till this >> > // point and there is an * in >> > // the beginning, the rule >> > //does not detect a structure. >> > mutex_lock >> > ( >> > - e1 >> > + &i->lock //this gives a parsing error >> > ); >> > >> > >> > > julia >> > > > > Gargi, > > Please insert the new lock above the __cacheline_aligned struct > member. I will do that, but is there any reason why the lock should be above ____cacheline_aligned struct member? > > I like the automated editing because it certainly cuts down on typos, > and even with this one, you can just move the new lock field upon > inspection. Sadly, I don't think the script will apply too widely > because the use of "_state" for driver global data isn't a standard, > nor availability of that struct in the functions needing the locking. I only saw three files that had the mlock and they followed the structure with _state as suffix and only had mutex_lock for &indio_dev->mlock. Hence, I went ahead with the script even though I knew it wasn't probably very safe. > > That sadness expressed ;) we do have a boatload of drivers upstream > that will eventually make this migration, so the value of the cocci > script would go beyond these 15 staging drivers. That's valuable. An autmoated script can work if there is a pattern all the file containing the mlocks possess. I believe for the staging driver all the structures have a _state as prefix and the mutex lock is held on those structures. > > Let's make sure anything done with any automated tools is walked > through thoroughly. Yes of course! My intention for a script was to make the transition to private locks a little less hassle free if possible. :) Thanks! Gargi Per the task description: this is not intended > as a search&replace exercise. > > alisons > > >> > >> >> > >> Signed-off-by: Gargi Sharma <gs051095@gmail.com> >> > >> --- >> > >> drivers/staging/iio/adc/ad7280a.c | 21 +++++++++++---------- >> > >> 1 file changed, 11 insertions(+), 10 deletions(-) >> > >> >> > >> diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c >> > >> index ee679ac..27a3ce3 100644 >> > >> --- a/drivers/staging/iio/adc/ad7280a.c >> > >> +++ b/drivers/staging/iio/adc/ad7280a.c >> > >> @@ -136,6 +136,7 @@ struct ad7280_state { >> > >> unsigned char cb_mask[AD7280A_MAX_CHAIN]; >> > >> >> > >> __be32 buf[2] ____cacheline_aligned; >> > >> + struct mutex lock; /* protect sensor state */ >> > >> }; >> > >> >> > >> static void ad7280_crc8_build_table(unsigned char *crc_tab) >> > >> @@ -410,7 +411,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev, >> > >> devaddr = this_attr->address >> 8; >> > >> ch = this_attr->address & 0xFF; >> > >> >> > >> - mutex_lock(&indio_dev->mlock); >> > >> + mutex_lock(&st->lock); >> > >> if (readin) >> > >> st->cb_mask[devaddr] |= 1 << (ch + 2); >> > >> else >> > >> @@ -418,7 +419,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev, >> > >> >> > >> ret = ad7280_write(st, devaddr, AD7280A_CELL_BALANCE, >> > >> 0, st->cb_mask[devaddr]); >> > >> - mutex_unlock(&indio_dev->mlock); >> > >> + mutex_unlock(&st->lock); >> > >> >> > >> return ret ? ret : len; >> > >> } >> > >> @@ -433,10 +434,10 @@ static ssize_t ad7280_show_balance_timer(struct device *dev, >> > >> int ret; >> > >> unsigned int msecs; >> > >> >> > >> - mutex_lock(&indio_dev->mlock); >> > >> + mutex_lock(&st->lock); >> > >> ret = ad7280_read(st, this_attr->address >> 8, >> > >> this_attr->address & 0xFF); >> > >> - mutex_unlock(&indio_dev->mlock); >> > >> + mutex_unlock(&st->lock); >> > >> >> > >> if (ret < 0) >> > >> return ret; >> > >> @@ -466,11 +467,11 @@ static ssize_t ad7280_store_balance_timer(struct device *dev, >> > >> if (val > 31) >> > >> return -EINVAL; >> > >> >> > >> - mutex_lock(&indio_dev->mlock); >> > >> + mutex_lock(&st->lock); >> > >> ret = ad7280_write(st, this_attr->address >> 8, >> > >> this_attr->address & 0xFF, >> > >> 0, (val & 0x1F) << 3); >> > >> - mutex_unlock(&indio_dev->mlock); >> > >> + mutex_unlock(&st->lock); >> > >> >> > >> return ret ? ret : len; >> > >> } >> > >> @@ -655,7 +656,7 @@ static ssize_t ad7280_write_channel_config(struct device *dev, >> > >> >> > >> val = clamp(val, 0L, 0xFFL); >> > >> >> > >> - mutex_lock(&indio_dev->mlock); >> > >> + mutex_lock(&st->lock); >> > >> switch ((u32)this_attr->address) { >> > >> case AD7280A_CELL_OVERVOLTAGE: >> > >> st->cell_threshhigh = val; >> > >> @@ -674,7 +675,7 @@ static ssize_t ad7280_write_channel_config(struct device *dev, >> > >> ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, >> > >> this_attr->address, 1, val); >> > >> >> > >> - mutex_unlock(&indio_dev->mlock); >> > >> + mutex_unlock(&st->lock); >> > >> >> > >> return ret ? ret : len; >> > >> } >> > >> @@ -792,13 +793,13 @@ static int ad7280_read_raw(struct iio_dev *indio_dev, >> > >> >> > >> switch (m) { >> > >> case IIO_CHAN_INFO_RAW: >> > >> - mutex_lock(&indio_dev->mlock); >> > >> + mutex_lock(&st->lock); >> > >> if (chan->address == AD7280A_ALL_CELLS) >> > >> ret = ad7280_read_all_channels(st, st->scan_cnt, NULL); >> > >> else >> > >> ret = ad7280_read_channel(st, chan->address >> 8, >> > >> chan->address & 0xFF); >> > >> - mutex_unlock(&indio_dev->mlock); >> > >> + mutex_unlock(&st->lock); >> > >> >> > >> if (ret < 0) >> > >> return ret; >> > >> -- >> > >> 2.7.4 >> > >> >> > >> -- >> > >> >> -- >> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. >> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com. >> To post to this group, send email to outreachy-kernel@googlegroups.com. >> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/alpine.DEB.2.20.1703122235320.7649%40hadrien. >> For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock 2017-03-13 8:40 ` Gargi Sharma @ 2017-03-13 20:40 ` Julia Lawall 2017-03-13 20:50 ` Jonathan Cameron 2017-03-13 20:51 ` Jonathan Cameron 1 sibling, 1 reply; 22+ messages in thread From: Julia Lawall @ 2017-03-13 20:40 UTC (permalink / raw) To: Gargi Sharma Cc: Alison Schofield, Julia Lawall, outreachy-kernel, lars, Michael.Hennerich, jic23, knaack.h, Peter Meerwald-Stadler, linux-iio > > I like the automated editing because it certainly cuts down on typos, > > and even with this one, you can just move the new lock field upon > > inspection. Sadly, I don't think the script will apply too widely > > because the use of "_state" for driver global data isn't a standard, > > nor availability of that struct in the functions needing the locking. > I only saw three files that had the mlock and they followed the structure > with _state as suffix and only had mutex_lock for &indio_dev->mlock. > Hence, I went ahead with the script even though I knew it wasn't probably > very safe. If _state is useful, you can add a regular expression to the metavariable declaration to checkk for that. I think it would be something like: identifier x =~ "_state$"; but I don't actually know. I would prefer to use some more semantic concept to identify such a structure. For example, structures are often stored in other structures or passed to some generic function. Then you can get to the structure in a way that is based on its purpose rather than its name. julia ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock 2017-03-13 20:40 ` Julia Lawall @ 2017-03-13 20:50 ` Jonathan Cameron 2017-03-13 21:09 ` Julia Lawall 0 siblings, 1 reply; 22+ messages in thread From: Jonathan Cameron @ 2017-03-13 20:50 UTC (permalink / raw) To: Julia Lawall, Gargi Sharma Cc: Alison Schofield, outreachy-kernel, lars, Michael.Hennerich, knaack.h, Peter Meerwald-Stadler, linux-iio On 13/03/17 20:40, Julia Lawall wrote: >>> I like the automated editing because it certainly cuts down on typos, >>> and even with this one, you can just move the new lock field upon >>> inspection. Sadly, I don't think the script will apply too widely >>> because the use of "_state" for driver global data isn't a standard, >>> nor availability of that struct in the functions needing the locking. >> I only saw three files that had the mlock and they followed the structure >> with _state as suffix and only had mutex_lock for &indio_dev->mlock. >> Hence, I went ahead with the script even though I knew it wasn't probably >> very safe. > > If _state is useful, you can add a regular expression to the metavariable declaration to checkk for that. I think it would be something like: > > identifier x =~ "_state$"; > > but I don't actually know. I would prefer to use some more semantic > concept to identify such a structure. For example, structures are often > stored in other structures or passed to some generic function. Then you > can get to the structure in a way that is based on its purpose rather than > its name. > > julia > This particular one is almost always equal (possibly always) to what comes from the struct iio_dev a when iio_priv(a) is applied. Jonathan ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock 2017-03-13 20:50 ` Jonathan Cameron @ 2017-03-13 21:09 ` Julia Lawall 0 siblings, 0 replies; 22+ messages in thread From: Julia Lawall @ 2017-03-13 21:09 UTC (permalink / raw) To: Jonathan Cameron Cc: Gargi Sharma, Alison Schofield, outreachy-kernel, lars, Michael.Hennerich, knaack.h, Peter Meerwald-Stadler, linux-iio On Mon, 13 Mar 2017, Jonathan Cameron wrote: > On 13/03/17 20:40, Julia Lawall wrote: > >>> I like the automated editing because it certainly cuts down on typos, > >>> and even with this one, you can just move the new lock field upon > >>> inspection. Sadly, I don't think the script will apply too widely > >>> because the use of "_state" for driver global data isn't a standard, > >>> nor availability of that struct in the functions needing the locking. > >> I only saw three files that had the mlock and they followed the structure > >> with _state as suffix and only had mutex_lock for &indio_dev->mlock. > >> Hence, I went ahead with the script even though I knew it wasn't probably > >> very safe. > > > > If _state is useful, you can add a regular expression to the metavariable declaration to checkk for that. I think it would be something like: > > > > identifier x =~ "_state$"; > > > > but I don't actually know. I would prefer to use some more semantic > > concept to identify such a structure. For example, structures are often > > stored in other structures or passed to some generic function. Then you > > can get to the structure in a way that is based on its purpose rather than > > its name. > > > > julia > > > This particular one is almost always equal (possibly always) to what comes > from the struct iio_dev a when iio_priv(a) is applied. That seems like a promising lead. An issue with such assignments is that they might occur in a variable declaration or in the rest of the function body. If you write a pattern like the following: @@ struct foo *x; @@ *x = y() then it will match all of: struct foo *a = y(); b = y(); // where b has the right type if ((b = y()) < 0) return; m = b = y(); but not b = m = y(); In this case, you want there to be a path from this initialization to the thing you want to change. And you want to change all occurrences that fit some pattern, so the basic outline would be: *x = y() <... - x + z ...> julia ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock 2017-03-13 8:40 ` Gargi Sharma 2017-03-13 20:40 ` Julia Lawall @ 2017-03-13 20:51 ` Jonathan Cameron 2017-03-13 20:54 ` Lars-Peter Clausen 1 sibling, 1 reply; 22+ messages in thread From: Jonathan Cameron @ 2017-03-13 20:51 UTC (permalink / raw) To: Gargi Sharma, Alison Schofield Cc: Julia Lawall, outreachy-kernel, lars, Michael.Hennerich, knaack.h, Peter Meerwald-Stadler, linux-iio On 13/03/17 08:40, Gargi Sharma wrote: > On Mon, Mar 13, 2017 at 3:43 AM, Alison Schofield <amsfield22@gmail.com> wrote: >> On Sun, Mar 12, 2017 at 10:37:39PM +0100, Julia Lawall wrote: >>> >>> >>> On Mon, 13 Mar 2017, Gargi Sharma wrote: >>> >>>> On Mon, Mar 13, 2017 at 2:26 AM, Julia Lawall <julia.lawall@lip6.fr> wrote: >>>>> >>>>> >>>>> On Mon, 13 Mar 2017, Gargi Sharma wrote: >>>>> >>>>>> The IIO subsystem is redefining iio_dev->mlock to be used by >>>>>> the IIO core only for protecting device operating mode changes. >>>>>> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes. >>>>>> >>>>>> In this driver, mlock was being used to protect hardware state >>>>>> changes. Replace it with a lock in the devices global data. >>>>>> This was done using Coccinelle Script. >>>>>> >>>>>> @r1@ >>>>>> identifier xxx_state; >>>>>> @@ >>>>>> struct xxx_state { >>>>>> ... >>>>>> + struct mutex lock; /* protect sensor state */ >>>>>> }; >>>>>> >>>>>> @r2@ >>>>>> expression e; >>>>>> @@ >>>>>> - mutex_lock(e); >>>>>> + mutex_lock(&st->lock); >>>>>> >>>>>> @r3@ >>>>>> expression e; >>>>>> @@ >>>>>> - mutex_unlock(e); >>>>>> + mutex_unlock(&st->lock); >>>>> >>>>> >>>>> This rule is probably hjelpful in practice, but is very unsafe. Is there >>>>> any way that you can characterize the kind of structure you want to add a >>>>> new lock to? And likewise, what lock and unlock calls you want to >>>>> replace? And what is st? >>>>> >>>> Hi! >>>> >>>> I wanted to do the following things, but was getting a parse error with the >>>> Coccinelle Script. >>>> >>>> st is a structure of type xxx_state. If I inherit xxx_state from r1, >>>> to r2 and write >>>> the following in the cocci script, for some reason it was not parsing >>>> the structure at all. >>>> >>>> @r2@ >>>> expression e1,e2; >>>> xxx_state << r1.xxx_state; >>> >>> << is the way you inherit a metavariable in script code. The first script >>> language available was python and it was considered to be odd to ask >>> people to put types on variables to be used in python code. In any case, >>> all kinds of terms are represented as strings, so the tpye would not be >>> very useful. >>> >>> When you are in pattern matching (SmPL) code, metavariables are inherited >>> as >>> >>> identifier r1.xxx_state; >>> >>> (or whatever is the appropriate type) >>> >>> julia >>> >>>> identifier i; >>>> position p; >>>> @@ >>>> struct xxx_state i@p = e1; //If the script is only till this >>>> // point and there is an * in >>>> // the beginning, the rule >>>> //does not detect a structure. >>>> mutex_lock >>>> ( >>>> - e1 >>>> + &i->lock //this gives a parsing error >>>> ); >>>> >>>> >>>>> julia >>>>> >> >> Gargi, >> >> Please insert the new lock above the __cacheline_aligned struct >> member. > > I will do that, but is there any reason why the lock should be above > ____cacheline_aligned struct member? Yes. It's in fact very important that nothing comes after that. Will leave the why as an exercise for the reader. I'll give the hit of spi drivers that do DMA... > >> >> I like the automated editing because it certainly cuts down on typos, >> and even with this one, you can just move the new lock field upon >> inspection. Sadly, I don't think the script will apply too widely >> because the use of "_state" for driver global data isn't a standard, >> nor availability of that struct in the functions needing the locking. > I only saw three files that had the mlock and they followed the structure > with _state as suffix and only had mutex_lock for &indio_dev->mlock. > Hence, I went ahead with the script even though I knew it wasn't probably > very safe. >> >> That sadness expressed ;) we do have a boatload of drivers upstream >> that will eventually make this migration, so the value of the cocci >> script would go beyond these 15 staging drivers. That's valuable. > > An autmoated script can work if there is a pattern all the file containing > the mlocks possess. I believe for the staging driver all the structures > have a _state as prefix and the mutex lock is held on those structures. >> >> Let's make sure anything done with any automated tools is walked >> through thoroughly. > Yes of course! My intention for a script was to make the transition to > private locks a little less hassle free if possible. :) > > Thanks! > Gargi > Per the task description: this is not intended >> as a search&replace exercise. >> >> alisons >> >> >>>>>> >>>>>> Signed-off-by: Gargi Sharma <gs051095@gmail.com> >>>>>> --- >>>>>> drivers/staging/iio/adc/ad7280a.c | 21 +++++++++++---------- >>>>>> 1 file changed, 11 insertions(+), 10 deletions(-) >>>>>> >>>>>> diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c >>>>>> index ee679ac..27a3ce3 100644 >>>>>> --- a/drivers/staging/iio/adc/ad7280a.c >>>>>> +++ b/drivers/staging/iio/adc/ad7280a.c >>>>>> @@ -136,6 +136,7 @@ struct ad7280_state { >>>>>> unsigned char cb_mask[AD7280A_MAX_CHAIN]; >>>>>> >>>>>> __be32 buf[2] ____cacheline_aligned; >>>>>> + struct mutex lock; /* protect sensor state */ >>>>>> }; >>>>>> >>>>>> static void ad7280_crc8_build_table(unsigned char *crc_tab) >>>>>> @@ -410,7 +411,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev, >>>>>> devaddr = this_attr->address >> 8; >>>>>> ch = this_attr->address & 0xFF; >>>>>> >>>>>> - mutex_lock(&indio_dev->mlock); >>>>>> + mutex_lock(&st->lock); >>>>>> if (readin) >>>>>> st->cb_mask[devaddr] |= 1 << (ch + 2); >>>>>> else >>>>>> @@ -418,7 +419,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev, >>>>>> >>>>>> ret = ad7280_write(st, devaddr, AD7280A_CELL_BALANCE, >>>>>> 0, st->cb_mask[devaddr]); >>>>>> - mutex_unlock(&indio_dev->mlock); >>>>>> + mutex_unlock(&st->lock); >>>>>> >>>>>> return ret ? ret : len; >>>>>> } >>>>>> @@ -433,10 +434,10 @@ static ssize_t ad7280_show_balance_timer(struct device *dev, >>>>>> int ret; >>>>>> unsigned int msecs; >>>>>> >>>>>> - mutex_lock(&indio_dev->mlock); >>>>>> + mutex_lock(&st->lock); >>>>>> ret = ad7280_read(st, this_attr->address >> 8, >>>>>> this_attr->address & 0xFF); >>>>>> - mutex_unlock(&indio_dev->mlock); >>>>>> + mutex_unlock(&st->lock); >>>>>> >>>>>> if (ret < 0) >>>>>> return ret; >>>>>> @@ -466,11 +467,11 @@ static ssize_t ad7280_store_balance_timer(struct device *dev, >>>>>> if (val > 31) >>>>>> return -EINVAL; >>>>>> >>>>>> - mutex_lock(&indio_dev->mlock); >>>>>> + mutex_lock(&st->lock); >>>>>> ret = ad7280_write(st, this_attr->address >> 8, >>>>>> this_attr->address & 0xFF, >>>>>> 0, (val & 0x1F) << 3); >>>>>> - mutex_unlock(&indio_dev->mlock); >>>>>> + mutex_unlock(&st->lock); >>>>>> >>>>>> return ret ? ret : len; >>>>>> } >>>>>> @@ -655,7 +656,7 @@ static ssize_t ad7280_write_channel_config(struct device *dev, >>>>>> >>>>>> val = clamp(val, 0L, 0xFFL); >>>>>> >>>>>> - mutex_lock(&indio_dev->mlock); >>>>>> + mutex_lock(&st->lock); >>>>>> switch ((u32)this_attr->address) { >>>>>> case AD7280A_CELL_OVERVOLTAGE: >>>>>> st->cell_threshhigh = val; >>>>>> @@ -674,7 +675,7 @@ static ssize_t ad7280_write_channel_config(struct device *dev, >>>>>> ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, >>>>>> this_attr->address, 1, val); >>>>>> >>>>>> - mutex_unlock(&indio_dev->mlock); >>>>>> + mutex_unlock(&st->lock); >>>>>> >>>>>> return ret ? ret : len; >>>>>> } >>>>>> @@ -792,13 +793,13 @@ static int ad7280_read_raw(struct iio_dev *indio_dev, >>>>>> >>>>>> switch (m) { >>>>>> case IIO_CHAN_INFO_RAW: >>>>>> - mutex_lock(&indio_dev->mlock); >>>>>> + mutex_lock(&st->lock); >>>>>> if (chan->address == AD7280A_ALL_CELLS) >>>>>> ret = ad7280_read_all_channels(st, st->scan_cnt, NULL); >>>>>> else >>>>>> ret = ad7280_read_channel(st, chan->address >> 8, >>>>>> chan->address & 0xFF); >>>>>> - mutex_unlock(&indio_dev->mlock); >>>>>> + mutex_unlock(&st->lock); >>>>>> >>>>>> if (ret < 0) >>>>>> return ret; >>>>>> -- >>>>>> 2.7.4 >>>>>> >>>>>> -- >>>> >>> >>> -- >>> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. >>> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com. >>> To post to this group, send email to outreachy-kernel@googlegroups.com. >>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/alpine.DEB.2.20.1703122235320.7649%40hadrien. >>> For more options, visit https://groups.google.com/d/optout. > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock 2017-03-13 20:51 ` Jonathan Cameron @ 2017-03-13 20:54 ` Lars-Peter Clausen 2017-03-13 21:25 ` Jonathan Cameron 2017-03-14 17:36 ` Gargi Sharma 0 siblings, 2 replies; 22+ messages in thread From: Lars-Peter Clausen @ 2017-03-13 20:54 UTC (permalink / raw) To: Jonathan Cameron, Gargi Sharma, Alison Schofield Cc: Julia Lawall, outreachy-kernel, Michael.Hennerich, knaack.h, Peter Meerwald-Stadler, linux-iio On 03/13/2017 09:51 PM, Jonathan Cameron wrote: [...] >>> Gargi, >>> >>> Please insert the new lock above the __cacheline_aligned struct >>> member. >> >> I will do that, but is there any reason why the lock should be above >> ____cacheline_aligned struct member? > Yes. It's in fact very important that nothing comes after that. > > Will leave the why as an exercise for the reader. I'll give the > hit of spi drivers that do DMA... One hint though, the answer is somewhere in Documentation/DMA-API.txt ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock 2017-03-13 20:54 ` Lars-Peter Clausen @ 2017-03-13 21:25 ` Jonathan Cameron 2017-03-14 17:36 ` Gargi Sharma 1 sibling, 0 replies; 22+ messages in thread From: Jonathan Cameron @ 2017-03-13 21:25 UTC (permalink / raw) To: Lars-Peter Clausen, Gargi Sharma, Alison Schofield Cc: Julia Lawall, outreachy-kernel, Michael.Hennerich, knaack.h, Peter Meerwald-Stadler, linux-iio On 13/03/17 20:54, Lars-Peter Clausen wrote: > On 03/13/2017 09:51 PM, Jonathan Cameron wrote: > [...] >>>> Gargi, >>>> >>>> Please insert the new lock above the __cacheline_aligned struct >>>> member. >>> >>> I will do that, but is there any reason why the lock should be above >>> ____cacheline_aligned struct member? >> Yes. It's in fact very important that nothing comes after that. >> >> Will leave the why as an exercise for the reader. I'll give the >> hit of spi drivers that do DMA... > > > One hint though, the answer is somewhere in Documentation/DMA-API.txt Took me a while to come across this one when I first started writing SPI drivers ;) > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock 2017-03-13 20:54 ` Lars-Peter Clausen @ 2017-03-14 17:36 ` Gargi Sharma 2017-03-14 17:36 ` Gargi Sharma 1 sibling, 0 replies; 22+ messages in thread From: Gargi Sharma @ 2017-03-14 17:36 UTC (permalink / raw) To: Lars-Peter Clausen Cc: Jonathan Cameron, Alison Schofield, Julia Lawall, outreachy-kernel, Michael.Hennerich, knaack.h, Peter Meerwald-Stadler, linux-iio On Tue, Mar 14, 2017 at 2:24 AM, Lars-Peter Clausen <lars@metafoo.de> wrote: > On 03/13/2017 09:51 PM, Jonathan Cameron wrote: > [...] >>>> Gargi, >>>> >>>> Please insert the new lock above the __cacheline_aligned struct >>>> member. >>> >>> I will do that, but is there any reason why the lock should be above >>> ____cacheline_aligned struct member? >> Yes. It's in fact very important that nothing comes after that. >> >> Will leave the why as an exercise for the reader. I'll give the >> hit of spi drivers that do DMA... > > > One hint though, the answer is somewhere in Documentation/DMA-API.txt >From what I have understood is that cacheline is used for keeping most frequently accessed values in adjacent cachelines. We want the cacheline_aligned at the end in the struct definition so as to avoid any holes after the cacheline boundary. > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock @ 2017-03-14 17:36 ` Gargi Sharma 0 siblings, 0 replies; 22+ messages in thread From: Gargi Sharma @ 2017-03-14 17:36 UTC (permalink / raw) To: Lars-Peter Clausen Cc: Jonathan Cameron, Alison Schofield, Julia Lawall, outreachy-kernel, Michael.Hennerich, knaack.h, Peter Meerwald-Stadler, linux-iio On Tue, Mar 14, 2017 at 2:24 AM, Lars-Peter Clausen <lars@metafoo.de> wrote: > On 03/13/2017 09:51 PM, Jonathan Cameron wrote: > [...] >>>> Gargi, >>>> >>>> Please insert the new lock above the __cacheline_aligned struct >>>> member. >>> >>> I will do that, but is there any reason why the lock should be above >>> ____cacheline_aligned struct member? >> Yes. It's in fact very important that nothing comes after that. >> >> Will leave the why as an exercise for the reader. I'll give the >> hit of spi drivers that do DMA... > > > One hint though, the answer is somewhere in Documentation/DMA-API.txt From what I have understood is that cacheline is used for keeping most frequently accessed values in adjacent cachelines. We want the cacheline_aligned at the end in the struct definition so as to avoid any holes after the cacheline boundary. > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock 2017-03-14 17:36 ` Gargi Sharma @ 2017-03-14 22:44 ` Jonathan Cameron -1 siblings, 0 replies; 22+ messages in thread From: Jonathan Cameron @ 2017-03-14 22:44 UTC (permalink / raw) To: Gargi Sharma, Lars-Peter Clausen Cc: Jonathan Cameron, Alison Schofield, Julia Lawall, outreachy-kernel, Michael.Hennerich, knaack.h, Peter Meerwald-Stadler, linux-iio On 14 March 2017 17:36:16 GMT+00:00, Gargi Sharma <gs051095@gmail=2Ecom> w= rote: >On Tue, Mar 14, 2017 at 2:24 AM, Lars-Peter Clausen <lars@metafoo=2Ede> >wrote: >> On 03/13/2017 09:51 PM, Jonathan Cameron wrote: >> [=2E=2E=2E] >>>>> Gargi, >>>>> >>>>> Please insert the new lock above the __cacheline_aligned struct >>>>> member=2E >>>> >>>> I will do that, but is there any reason why the lock should be >above >>>> ____cacheline_aligned struct member? >>> Yes=2E It's in fact very important that nothing comes after that=2E >>> >>> Will leave the why as an exercise for the reader=2E I'll give the >>> hit of spi drivers that do DMA=2E=2E=2E >> >> >> One hint though, the answer is somewhere in Documentation/DMA-API=2Etxt > >>From what I have understood is that cacheline is used for keeping most >frequently accessed values in adjacent cachelines=2E=20 Why? What does cache line actually mean? > We want the >cacheline_aligned at the end in the struct definition so as to avoid >any holes after the cacheline boundary=2E Nope=2E Try http://www=2Epebblebay=2Ecom/a-guide-to-using-direct-memory-access-in-embe= dded-systems-part-two/ (Hint, search for 'shares') >> >-- >To unsubscribe from this list: send the line "unsubscribe linux-iio" in >the body of a message to majordomo@vger=2Ekernel=2Eorg >More majordomo info at http://vger=2Ekernel=2Eorg/majordomo-info=2Ehtml --=20 Sent from my Android device with K-9 Mail=2E Please excuse my brevity=2E ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock @ 2017-03-14 22:44 ` Jonathan Cameron 0 siblings, 0 replies; 22+ messages in thread From: Jonathan Cameron @ 2017-03-14 22:44 UTC (permalink / raw) To: Gargi Sharma, Lars-Peter Clausen Cc: Jonathan Cameron, Alison Schofield, Julia Lawall, outreachy-kernel, Michael.Hennerich, knaack.h, Peter Meerwald-Stadler, linux-iio On 14 March 2017 17:36:16 GMT+00:00, Gargi Sharma <gs051095@gmail.com> wrote: >On Tue, Mar 14, 2017 at 2:24 AM, Lars-Peter Clausen <lars@metafoo.de> >wrote: >> On 03/13/2017 09:51 PM, Jonathan Cameron wrote: >> [...] >>>>> Gargi, >>>>> >>>>> Please insert the new lock above the __cacheline_aligned struct >>>>> member. >>>> >>>> I will do that, but is there any reason why the lock should be >above >>>> ____cacheline_aligned struct member? >>> Yes. It's in fact very important that nothing comes after that. >>> >>> Will leave the why as an exercise for the reader. I'll give the >>> hit of spi drivers that do DMA... >> >> >> One hint though, the answer is somewhere in Documentation/DMA-API.txt > >>From what I have understood is that cacheline is used for keeping most >frequently accessed values in adjacent cachelines. Why? What does cache line actually mean? > We want the >cacheline_aligned at the end in the struct definition so as to avoid >any holes after the cacheline boundary. Nope. Try http://www.pebblebay.com/a-guide-to-using-direct-memory-access-in-embedded-systems-part-two/ (Hint, search for 'shares') >> >-- >To unsubscribe from this list: send the line "unsubscribe linux-iio" in >the body of a message to majordomo@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html -- Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock 2017-03-14 22:44 ` Jonathan Cameron (?) @ 2017-03-15 18:15 ` Gargi Sharma 2017-03-16 17:41 ` Alison Schofield -1 siblings, 1 reply; 22+ messages in thread From: Gargi Sharma @ 2017-03-15 18:15 UTC (permalink / raw) To: Jonathan Cameron Cc: Lars-Peter Clausen, Jonathan Cameron, Alison Schofield, Julia Lawall, outreachy-kernel, Michael.Hennerich, knaack.h, Peter Meerwald-Stadler, linux-iio On Wed, Mar 15, 2017 at 4:14 AM, Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote: > > > On 14 March 2017 17:36:16 GMT+00:00, Gargi Sharma <gs051095@gmail.com> wrote: >>On Tue, Mar 14, 2017 at 2:24 AM, Lars-Peter Clausen <lars@metafoo.de> >>wrote: >>> On 03/13/2017 09:51 PM, Jonathan Cameron wrote: >>> [...] >>>>>> Gargi, >>>>>> >>>>>> Please insert the new lock above the __cacheline_aligned struct >>>>>> member. >>>>> >>>>> I will do that, but is there any reason why the lock should be >>above >>>>> ____cacheline_aligned struct member? >>>> Yes. It's in fact very important that nothing comes after that. >>>> >>>> Will leave the why as an exercise for the reader. I'll give the >>>> hit of spi drivers that do DMA... >>> >>> >>> One hint though, the answer is somewhere in Documentation/DMA-API.txt >> > >From what I have understood is that cacheline is used for keeping most >>frequently accessed values in adjacent cachelines. > > Why? What does cache line actually mean? > >> We want the >>cacheline_aligned at the end in the struct definition so as to avoid >>any holes after the cacheline boundary. > > Nope. Try > http://www.pebblebay.com/a-guide-to-using-direct-memory-access-in-embedded-systems-part-two/ > > (Hint, search for 'shares') Okay, I got two things from the article: 1. Hardware that uses DMA reads faster if the data is aligned at 16/32/64B boundaries. 2. You want to align buffer being accessed for DMa transfers to prevent cache incoherence. What I still don't understand is why nothing should come after __cacheline_aligned struct member? Thanks, Gargi > >>> >>-- >>To unsubscribe from this list: send the line "unsubscribe linux-iio" in >>the body of a message to majordomo@vger.kernel.org >>More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock 2017-03-15 18:15 ` Gargi Sharma @ 2017-03-16 17:41 ` Alison Schofield 2017-03-16 17:45 ` Alison Schofield 0 siblings, 1 reply; 22+ messages in thread From: Alison Schofield @ 2017-03-16 17:41 UTC (permalink / raw) To: Gargi Sharma Cc: Jonathan Cameron, Lars-Peter Clausen, Jonathan Cameron, Julia Lawall, outreachy-kernel, Michael.Hennerich, knaack.h, Peter Meerwald-Stadler, linux-iio On Wed, Mar 15, 2017 at 11:45:11PM +0530, Gargi Sharma wrote: > On Wed, Mar 15, 2017 at 4:14 AM, Jonathan Cameron > <jic23@jic23.retrosnub.co.uk> wrote: > > > > > > On 14 March 2017 17:36:16 GMT+00:00, Gargi Sharma <gs051095@gmail.com> wrote: > >>On Tue, Mar 14, 2017 at 2:24 AM, Lars-Peter Clausen <lars@metafoo.de> > >>wrote: > >>> On 03/13/2017 09:51 PM, Jonathan Cameron wrote: > >>> [...] > >>>>>> Gargi, > >>>>>> > >>>>>> Please insert the new lock above the __cacheline_aligned struct > >>>>>> member. > >>>>> > >>>>> I will do that, but is there any reason why the lock should be > >>above > >>>>> ____cacheline_aligned struct member? > >>>> Yes. It's in fact very important that nothing comes after that. > >>>> > >>>> Will leave the why as an exercise for the reader. I'll give the > >>>> hit of spi drivers that do DMA... > >>> > >>> > >>> One hint though, the answer is somewhere in Documentation/DMA-API.txt > >> > > >From what I have understood is that cacheline is used for keeping most > >>frequently accessed values in adjacent cachelines. > > > > Why? What does cache line actually mean? > > > >> We want the > >>cacheline_aligned at the end in the struct definition so as to avoid > >>any holes after the cacheline boundary. > > > > Nope. Try > > http://www.pebblebay.com/a-guide-to-using-direct-memory-access-in-embedded-systems-part-two/ > > > > (Hint, search for 'shares') > > Okay, I got two things from the article: > 1. Hardware that uses DMA reads faster if the data is aligned at > 16/32/64B boundaries. > 2. You want to align buffer being accessed for DMa transfers to > prevent cache incoherence. > > What I still don't understand is why nothing should come after > __cacheline_aligned struct member? > > Thanks, > Gargi Hi Gargi, Short answer to why nothing can come after, is because it could indeed share the cacheline. ___cacheline_aligned only guarantees alignment at the beginning of the cacheline. I've been following and reading the posted links. Here's another link, kind of ancient, but does a nice explanation of the process whereby that extra field, sharing your buffers cacheline, could lead to corruption of that field or the buffer data. https://lwn.net/Articles/2265/ I still haven't bottomed out on this though. I get the need to do this to be safe, but I'd like to understand how it fits in with systems that claim cache coherency. Is this a case those schemes cannot handle, or, are there MP systems that are just not cache incoherent? alisons > > > >>> > >>-- > >>To unsubscribe from this list: send the line "unsubscribe linux-iio" in > >>the body of a message to majordomo@vger.kernel.org > >>More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > > Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock 2017-03-16 17:41 ` Alison Schofield @ 2017-03-16 17:45 ` Alison Schofield 0 siblings, 0 replies; 22+ messages in thread From: Alison Schofield @ 2017-03-16 17:45 UTC (permalink / raw) To: Gargi Sharma Cc: Jonathan Cameron, Lars-Peter Clausen, Jonathan Cameron, Julia Lawall, outreachy-kernel, Michael.Hennerich, knaack.h, Peter Meerwald-Stadler, linux-iio On Thu, Mar 16, 2017 at 10:41:02AM -0700, Alison Schofield wrote: > On Wed, Mar 15, 2017 at 11:45:11PM +0530, Gargi Sharma wrote: > > On Wed, Mar 15, 2017 at 4:14 AM, Jonathan Cameron > > <jic23@jic23.retrosnub.co.uk> wrote: > > > > > > > > > On 14 March 2017 17:36:16 GMT+00:00, Gargi Sharma <gs051095@gmail.com> wrote: > > >>On Tue, Mar 14, 2017 at 2:24 AM, Lars-Peter Clausen <lars@metafoo.de> > > >>wrote: > > >>> On 03/13/2017 09:51 PM, Jonathan Cameron wrote: > > >>> [...] > > >>>>>> Gargi, > > >>>>>> > > >>>>>> Please insert the new lock above the __cacheline_aligned struct > > >>>>>> member. > > >>>>> > > >>>>> I will do that, but is there any reason why the lock should be > > >>above > > >>>>> ____cacheline_aligned struct member? > > >>>> Yes. It's in fact very important that nothing comes after that. > > >>>> > > >>>> Will leave the why as an exercise for the reader. I'll give the > > >>>> hit of spi drivers that do DMA... > > >>> > > >>> > > >>> One hint though, the answer is somewhere in Documentation/DMA-API.txt > > >> > > > >From what I have understood is that cacheline is used for keeping most > > >>frequently accessed values in adjacent cachelines. > > > > > > Why? What does cache line actually mean? > > > > > >> We want the > > >>cacheline_aligned at the end in the struct definition so as to avoid > > >>any holes after the cacheline boundary. > > > > > > Nope. Try > > > http://www.pebblebay.com/a-guide-to-using-direct-memory-access-in-embedded-systems-part-two/ > > > > > > (Hint, search for 'shares') > > > > Okay, I got two things from the article: > > 1. Hardware that uses DMA reads faster if the data is aligned at > > 16/32/64B boundaries. > > 2. You want to align buffer being accessed for DMa transfers to > > prevent cache incoherence. > > > > What I still don't understand is why nothing should come after > > __cacheline_aligned struct member? > > > > Thanks, > > Gargi > > Hi Gargi, > > Short answer to why nothing can come after, is because it could > indeed share the cacheline. ___cacheline_aligned only guarantees > alignment at the beginning of the cacheline. > > I've been following and reading the posted links. Here's another > link, kind of ancient, but does a nice explanation of the process > whereby that extra field, sharing your buffers cacheline, could > lead to corruption of that field or the buffer data. > https://lwn.net/Articles/2265/ > > I still haven't bottomed out on this though. I get the need > to do this to be safe, but I'd like to understand how it fits > in with systems that claim cache coherency. Is this a case > those schemes cannot handle, or, are there MP systems that > are just not cache incoherent? oops...a bit incoherent there. Meant to say - are just not cache coherent at all! ie..totally incoherent! > > alisons > > > > > >>> > > >>-- > > >>To unsubscribe from this list: send the line "unsubscribe linux-iio" in > > >>the body of a message to majordomo@vger.kernel.org > > >>More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > > -- > > > Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2017-03-16 17:45 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-12 20:45 [PATCH] staging: iio: adc: Replace mlock with driver private lock Gargi Sharma 2017-03-12 20:56 ` [Outreachy kernel] " Julia Lawall 2017-03-12 21:33 ` Gargi Sharma 2017-03-12 21:37 ` Julia Lawall 2017-03-12 22:13 ` Alison Schofield 2017-03-12 22:27 ` Julia Lawall 2017-03-13 8:41 ` Gargi Sharma 2017-03-15 9:44 ` Julia Lawall 2017-03-13 8:40 ` Gargi Sharma 2017-03-13 20:40 ` Julia Lawall 2017-03-13 20:50 ` Jonathan Cameron 2017-03-13 21:09 ` Julia Lawall 2017-03-13 20:51 ` Jonathan Cameron 2017-03-13 20:54 ` Lars-Peter Clausen 2017-03-13 21:25 ` Jonathan Cameron 2017-03-14 17:36 ` Gargi Sharma 2017-03-14 17:36 ` Gargi Sharma 2017-03-14 22:44 ` Jonathan Cameron 2017-03-14 22:44 ` Jonathan Cameron 2017-03-15 18:15 ` Gargi Sharma 2017-03-16 17:41 ` Alison Schofield 2017-03-16 17:45 ` Alison Schofield
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.