Re: [PATCH] tty/n_gsm.c: fix a memory leak in gsmld_open
于 2014年07月28日 17:03, xinhui.pan 写道:
> Hi, Jiri
>
> 于 2014年07月28日 16:49, Jiri Slaby 写道:
>> On 07/28/2014 10:14 AM, xinhui.pan wrote:
>>> If gsmld_attach_gsm fails, the gsm is not used anymore.
>>> tty core will not call gsmld_close to do the cleanup work.
>>> tty core just restore to the tty old ldisc.
>>> That always causes memory leak.
>>
>> Nice catch!
>>
>>> --- a/drivers/tty/n_gsm.c
>>> +++ b/drivers/tty/n_gsm.c
>>> @@ -2382,7 +2383,13 @@ static int gsmld_open(struct tty_struct *tty)
>>>
>>> /* Attach the initial passive connection */
>>> gsm->encoding = 1;
>>> - return gsmld_attach_gsm(tty, gsm);
>>> +
>>> + ret = gsmld_attach_gsm(tty, gsm);
>>> + if (ret != 0) {
>>> + gsm_cleanup_mux(gsm);
>>> + mux_put(gsm);
>>
>> It is quite illogical to put the mux here. It should be in gsmld_open.
>> I.e. gsm_cleanup_mux here, mux_put there.
>>
>
> Thanks for your reply :)
> But I am a little confused with your comments, could you explain it when you
> are free?
> Sorry for my poor English.
>
hi, Jiri
I guess you want gsm_cleanup_mux() called in gsmld_attach_gsm(), just
after gsm_activate_mux() fails?
Yes, that seems really make sence. :)
Thanks for pointing out it.
Let me explain my opinion. :)
Actually gsmld_attach_gsm results in two different effects when it fails.
(a)gsmld_attach_gsm -> gsm_activate_mux(gsm_mux[] is full,-EBUSY), then
gsm_mux[] did not change, and gsm->num is invaild;
(b)gsmld_attach_gsm -> gsm_activate_mux(return -ENOMEM), then gsm_mux[]
changes, and gsm->num is vaild.
To be honest, I am not very clear about this. I even suspect this is done in
such way on purpose.
So I just keep the code what it is now. Let's handle the error in gsmld_open().
We can make sure that gsm is not used here and it's safe to cleanup and put it.
thanks,
xinhui
> thanks,
>
> xinhui
>
>> thanks,
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty/n_gsm.c: fix a memory leak in gsmld_open
Hi, Jiri
于 2014年07月28日 16:49, Jiri Slaby 写道:
> On 07/28/2014 10:14 AM, xinhui.pan wrote:
>> If gsmld_attach_gsm fails, the gsm is not used anymore.
>> tty core will not call gsmld_close to do the cleanup work.
>> tty core just restore to the tty old ldisc.
>> That always causes memory leak.
>
> Nice catch!
>
>> --- a/drivers/tty/n_gsm.c
>> +++ b/drivers/tty/n_gsm.c
>> @@ -2382,7 +2383,13 @@ static int gsmld_open(struct tty_struct *tty)
>>
>> /* Attach the initial passive connection */
>> gsm->encoding = 1;
>> -return gsmld_attach_gsm(tty, gsm);
>> +
>> +ret = gsmld_attach_gsm(tty, gsm);
>> +if (ret != 0) {
>> +gsm_cleanup_mux(gsm);
>> +mux_put(gsm);
>
> It is quite illogical to put the mux here. It should be in gsmld_open.
> I.e. gsm_cleanup_mux here, mux_put there.
>
Thanks for your reply :)
But I am a little confused with your comments, could you explain it when you
are free?
Sorry for my poor English.
thanks,
xinhui
> thanks,
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty/n_gsm.c: fix a memory leak in gsmld_open
hi, All
thanks for reading. Below is my trigger.
#define WARN(x) do{ \
KLOG_ERROR("XINHUI",x); \
}while(0)
int main()
{
KLOG_ERROR("XINHUI", "hello world :)\n");
long fp[4];
int i = 0;
int next[4]={1,2,3,0};
do{
char path[64] = "dev/tty30";
path[strlen(path) - 1] = '0' + i;
fp[i] = open(path, O_RDWR);
if (fp[i] == -1){
WARN("mlk tty open fails\n");
return 0;
}
}while(++i < 4);
i = 0;
do{
int p = 21;
int ret = ioctl(fp[i], TIOCSETD , );
if (ret != 0) {
WARN("mlk tty ioctl fails\n");
}
i = next[i];
}while(1);
/* never run here, just ctrl^C */
return 0 ;
}
without this patch, running my own tests.
my result:
PROCRANK:
.
RAM: 1993076K total, 147968K free, 708K buffers, 108340K cached, 332K shmem,
1092232K slab
AWESOME!
cat /proc/slabinfo
.
kmalloc-2048 509700 509700 2048 168 : tunables000 :
slabdata 32285 32285 0
.....
AWESOME!!
thanks,
xinhui
于 2014年07月28日 16:14, xinhui.pan 写道:
> If gsmld_attach_gsm fails, the gsm is not used anymore.
> tty core will not call gsmld_close to do the cleanup work.
> tty core just restore to the tty old ldisc.
> That always causes memory leak.
>
> Signed-off-by: xinhui.pan
> Reported-by: Peter Hurley
> ---
> drivers/tty/n_gsm.c |9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 81e7ccb..6cb1a6d 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -2368,6 +2368,7 @@ static void gsmld_close(struct tty_struct *tty)
> static int gsmld_open(struct tty_struct *tty)
> {
> struct gsm_mux *gsm;
> + int ret;
>
> if (tty->ops->write == NULL)
> return -EINVAL;
> @@ -2382,7 +2383,13 @@ static int gsmld_open(struct tty_struct *tty)
>
> /* Attach the initial passive connection */
> gsm->encoding = 1;
> - return gsmld_attach_gsm(tty, gsm);
> +
> + ret = gsmld_attach_gsm(tty, gsm);
> + if (ret != 0) {
> + gsm_cleanup_mux(gsm);
> + mux_put(gsm);
> + }
> + return ret;
> }
>
> /**
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] tty/n_gsm.c: fix a memory leak in gsmld_open
If gsmld_attach_gsm fails, the gsm is not used anymore.
tty core will not call gsmld_close to do the cleanup work.
tty core just restore to the tty old ldisc.
That always causes memory leak.
Signed-off-by: xinhui.pan
Reported-by: Peter Hurley
---
drivers/tty/n_gsm.c |9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 81e7ccb..6cb1a6d 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2368,6 +2368,7 @@ static void gsmld_close(struct tty_struct *tty)
static int gsmld_open(struct tty_struct *tty)
{
struct gsm_mux *gsm;
+ int ret;
if (tty->ops->write == NULL)
return -EINVAL;
@@ -2382,7 +2383,13 @@ static int gsmld_open(struct tty_struct *tty)
/* Attach the initial passive connection */
gsm->encoding = 1;
- return gsmld_attach_gsm(tty, gsm);
+
+ ret = gsmld_attach_gsm(tty, gsm);
+ if (ret != 0) {
+ gsm_cleanup_mux(gsm);
+ mux_put(gsm);
+ }
+ return ret;
}
/**
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty/n_gsm.c: do not clear gsm_mux entry when the gsm is not closed
hi, Greg
于 2014年07月28日 02:09, Greg Kroah-Hartman 写道:
> On Thu, Jul 24, 2014 at 05:17:01PM +0800, xinhui.pan wrote:
>> If the gsmtty is still used by some process, we could not just
>> simply clear gsm_mux[gsm->num]. Clear it when gsm is being free.
>> Otherwise we will hit crashes when userspace close the gsmtty.
>>
>> Also add gsm_mux_get() and gsm_mux_put() to make gsm_mux[] is used safely.
>> We can do activation/deactivation with same gsm more than once now.
>> This is for fixing the FIXME.
>>
>> Signed-off-by: xinhui.pan
>> Reviewed-by: Zhang Yanmin
>> ---
>> drivers/tty/n_gsm.c | 84
>> ---
>> 1 file changed, 60 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
>> index 81e7ccb..290df56 100644
>> --- a/drivers/tty/n_gsm.c
>> +++ b/drivers/tty/n_gsm.c
>> @@ -2020,6 +2020,58 @@ static void gsm_error(struct gsm_mux *gsm,
>> }
>>
>> /**
>> + * gsm_mux_get - get/fill one entry in gsm_mux
>> + * @gsm: our gsm
>> + *
>> + * Although its name end with get, it don't inc ref-count actually.
>
> Then don't call it a 'get' function :(
>
Thanks for you nice advices!
I will change its name. This is really a confusable name.
>> + * get one entry is just like fill pte, first memory access will
>> + * cause page_fault, the next accesses don't. So do here.
>
> This doesn't make much sense to me, can you please explain it better?
>
>> + */
>> +
>
> blank line?
>
Sorry about that. But I notice the other function's introduce in n_gsm.c is
done in same way.
I just wanted to keep same code style.
OK, I will change it.
I think no blank line is better. :)
>> +static int gsm_mux_get(struct gsm_mux *gsm)
>> +{
>> +int i;
>> +
>> +if (gsm->num >= MAX_MUX) /* gsm is alloc by kzalloc, just be careful */
>> +return -EIO;
>
> -EIO?
>
Thanks for pointing out the mistake.
perhaps -EINVAL is better.
>> +if (gsm_mux[gsm->num] == gsm) /* We have already set gsm->num */
>> +return 0;
>> +
>> +spin_lock(_mux_lock);
>> +for (i = 0; i < MAX_MUX; i++) {
>> +if (gsm_mux[i] == NULL) {
>> +gsm->num = i;
>> +gsm_mux[i] = gsm;
>> +break;
>> +}
>> +}
>> +spin_unlock(_mux_lock);
>> +
>> +if (i == MAX_MUX)
>> +return -EBUSY;
>> +return 0;
>> +}
>> +
>> +/**
>> + * gsm_mux_put - put/clear one entry in gsm_mux
>> + * @gsm: our gsm
>> + *
>> + * Although its name end with put, it don't dec ref-count actually.
>> + * put one entry is just like clear pte, So do here.
>> + */
>> +
>> +static void gsm_mux_put(struct gsm_mux *gsm)
>> +{
>> +if (gsm->num >= MAX_MUX)
>> +return;
>> +
>> +spin_lock(_mux_lock);
>> +if (gsm_mux[gsm->num] == gsm)
>
> How can this not be true?
That is a big problem. let me explain it.
In current code, we add gsm into gsm_mux[] in gsm_activate_mux(). So if
gsm_activate_mux() fails when gsm_mux[] is full, gsm->num is invaild, it is
zero in fact.
So when will free this gsm, we have to check if gsm_mux[gsm->num] is really the
one we are deleting.
The scenario is like below.
gsmld_open -> gsmld_attach_gsm -> gsm_activate_mux(fails) -> -> mux_put ->
gsm_free_muxr -> gsm_mux_put(need check gsm_mux[gsm->num] == gsm).
Current code is a little hard to understand, So I suggest that move the *codes
that changes gsm_mux[]* into gsm_alloc_mux(), then we don't need this check
anymore.
Actually I have worked out a new patch with such idea.
And I am doing the normal tests set up by Intel test team and other trigger
tests set up by myself. :)
>
>> +gsm_mux[gsm->num] = NULL;
>> +spin_unlock(_mux_lock);
>> +}
>
> Why can't you do dynamic reference counting of your structure, that
> would allow you to get rid of your global array, right?
>
Thanks for your nice comments.
Struct gsm has a ref-count already. :)
And also adding a ref-count is a little hard to me. :(
This global array is used to keep tracking the gsms that stands for the
gsmttyXX.
and it can tell us if we can create a new gsm. :)
In gsm_init we set *gsm_tty_driver = alloc_tty_driver(256);*
thanks,
xinhui
> thanks,
>
> greg k-h
>
Your reply really helps us a lot.
thanks,
xinhui
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty/n_gsm.c: do not clear gsm_mux entry when the gsm is not closed
hi, Greg
于 2014年07月28日 02:09, Greg Kroah-Hartman 写道:
On Thu, Jul 24, 2014 at 05:17:01PM +0800, xinhui.pan wrote:
If the gsmtty is still used by some process, we could not just
simply clear gsm_mux[gsm-num]. Clear it when gsm is being free.
Otherwise we will hit crashes when userspace close the gsmtty.
Also add gsm_mux_get() and gsm_mux_put() to make gsm_mux[] is used safely.
We can do activation/deactivation with same gsm more than once now.
This is for fixing the FIXME.
Signed-off-by: xinhui.pan xinhuix@intel.com
Reviewed-by: Zhang Yanmin yanmin_zh...@linux.intel.com
---
drivers/tty/n_gsm.c | 84
---
1 file changed, 60 insertions(+), 24 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 81e7ccb..290df56 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2020,6 +2020,58 @@ static void gsm_error(struct gsm_mux *gsm,
}
/**
+ * gsm_mux_get - get/fill one entry in gsm_mux
+ * @gsm: our gsm
+ *
+ * Although its name end with get, it don't inc ref-count actually.
Then don't call it a 'get' function :(
Thanks for you nice advices!
I will change its name. This is really a confusable name.
+ * get one entry is just like fill pte, first memory access will
+ * cause page_fault, the next accesses don't. So do here.
This doesn't make much sense to me, can you please explain it better?
+ */
+
blank line?
Sorry about that. But I notice the other function's introduce in n_gsm.c is
done in same way.
I just wanted to keep same code style.
OK, I will change it.
I think no blank line is better. :)
+static int gsm_mux_get(struct gsm_mux *gsm)
+{
+int i;
+
+if (gsm-num = MAX_MUX) /* gsm is alloc by kzalloc, just be careful */
+return -EIO;
-EIO?
Thanks for pointing out the mistake.
perhaps -EINVAL is better.
+if (gsm_mux[gsm-num] == gsm) /* We have already set gsm-num */
+return 0;
+
+spin_lock(gsm_mux_lock);
+for (i = 0; i MAX_MUX; i++) {
+if (gsm_mux[i] == NULL) {
+gsm-num = i;
+gsm_mux[i] = gsm;
+break;
+}
+}
+spin_unlock(gsm_mux_lock);
+
+if (i == MAX_MUX)
+return -EBUSY;
+return 0;
+}
+
+/**
+ * gsm_mux_put - put/clear one entry in gsm_mux
+ * @gsm: our gsm
+ *
+ * Although its name end with put, it don't dec ref-count actually.
+ * put one entry is just like clear pte, So do here.
+ */
+
+static void gsm_mux_put(struct gsm_mux *gsm)
+{
+if (gsm-num = MAX_MUX)
+return;
+
+spin_lock(gsm_mux_lock);
+if (gsm_mux[gsm-num] == gsm)
How can this not be true?
That is a big problem. let me explain it.
In current code, we add gsm into gsm_mux[] in gsm_activate_mux(). So if
gsm_activate_mux() fails when gsm_mux[] is full, gsm-num is invaild, it is
zero in fact.
So when will free this gsm, we have to check if gsm_mux[gsm-num] is really the
one we are deleting.
The scenario is like below.
gsmld_open - gsmld_attach_gsm - gsm_activate_mux(fails) - - mux_put -
gsm_free_muxr - gsm_mux_put(need check gsm_mux[gsm-num] == gsm).
Current code is a little hard to understand, So I suggest that move the *codes
that changes gsm_mux[]* into gsm_alloc_mux(), then we don't need this check
anymore.
Actually I have worked out a new patch with such idea.
And I am doing the normal tests set up by Intel test team and other trigger
tests set up by myself. :)
+gsm_mux[gsm-num] = NULL;
+spin_unlock(gsm_mux_lock);
+}
Why can't you do dynamic reference counting of your structure, that
would allow you to get rid of your global array, right?
Thanks for your nice comments.
Struct gsm has a ref-count already. :)
And also adding a ref-count is a little hard to me. :(
This global array is used to keep tracking the gsms that stands for the
gsmttyXX.
and it can tell us if we can create a new gsm. :)
In gsm_init we set *gsm_tty_driver = alloc_tty_driver(256);*
thanks,
xinhui
thanks,
greg k-h
Your reply really helps us a lot.
thanks,
xinhui
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] tty/n_gsm.c: fix a memory leak in gsmld_open
If gsmld_attach_gsm fails, the gsm is not used anymore.
tty core will not call gsmld_close to do the cleanup work.
tty core just restore to the tty old ldisc.
That always causes memory leak.
Signed-off-by: xinhui.pan xinhuix@intel.com
Reported-by: Peter Hurley pe...@hurleysoftware.com
---
drivers/tty/n_gsm.c |9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 81e7ccb..6cb1a6d 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2368,6 +2368,7 @@ static void gsmld_close(struct tty_struct *tty)
static int gsmld_open(struct tty_struct *tty)
{
struct gsm_mux *gsm;
+ int ret;
if (tty-ops-write == NULL)
return -EINVAL;
@@ -2382,7 +2383,13 @@ static int gsmld_open(struct tty_struct *tty)
/* Attach the initial passive connection */
gsm-encoding = 1;
- return gsmld_attach_gsm(tty, gsm);
+
+ ret = gsmld_attach_gsm(tty, gsm);
+ if (ret != 0) {
+ gsm_cleanup_mux(gsm);
+ mux_put(gsm);
+ }
+ return ret;
}
/**
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty/n_gsm.c: fix a memory leak in gsmld_open
hi, All
thanks for reading. Below is my trigger.
#define WARN(x) do{ \
KLOG_ERROR(XINHUI,x); \
}while(0)
int main()
{
KLOG_ERROR(XINHUI, hello world :)\n);
long fp[4];
int i = 0;
int next[4]={1,2,3,0};
do{
char path[64] = dev/tty30;
path[strlen(path) - 1] = '0' + i;
fp[i] = open(path, O_RDWR);
if (fp[i] == -1){
WARN(mlk tty open fails\n);
return 0;
}
}while(++i 4);
i = 0;
do{
int p = 21;
int ret = ioctl(fp[i], TIOCSETD , p);
if (ret != 0) {
WARN(mlk tty ioctl fails\n);
}
i = next[i];
}while(1);
/* never run here, just ctrl^C */
return 0 ;
}
without this patch, running my own tests.
my result:
PROCRANK:
.
RAM: 1993076K total, 147968K free, 708K buffers, 108340K cached, 332K shmem,
1092232K slab
AWESOME!
cat /proc/slabinfo
.
kmalloc-2048 509700 509700 2048 168 : tunables000 :
slabdata 32285 32285 0
.
AWESOME!!
thanks,
xinhui
于 2014年07月28日 16:14, xinhui.pan 写道:
If gsmld_attach_gsm fails, the gsm is not used anymore.
tty core will not call gsmld_close to do the cleanup work.
tty core just restore to the tty old ldisc.
That always causes memory leak.
Signed-off-by: xinhui.pan xinhuix@intel.com
Reported-by: Peter Hurley pe...@hurleysoftware.com
---
drivers/tty/n_gsm.c |9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 81e7ccb..6cb1a6d 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2368,6 +2368,7 @@ static void gsmld_close(struct tty_struct *tty)
static int gsmld_open(struct tty_struct *tty)
{
struct gsm_mux *gsm;
+ int ret;
if (tty-ops-write == NULL)
return -EINVAL;
@@ -2382,7 +2383,13 @@ static int gsmld_open(struct tty_struct *tty)
/* Attach the initial passive connection */
gsm-encoding = 1;
- return gsmld_attach_gsm(tty, gsm);
+
+ ret = gsmld_attach_gsm(tty, gsm);
+ if (ret != 0) {
+ gsm_cleanup_mux(gsm);
+ mux_put(gsm);
+ }
+ return ret;
}
/**
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty/n_gsm.c: fix a memory leak in gsmld_open
Hi, Jiri
于 2014年07月28日 16:49, Jiri Slaby 写道:
On 07/28/2014 10:14 AM, xinhui.pan wrote:
If gsmld_attach_gsm fails, the gsm is not used anymore.
tty core will not call gsmld_close to do the cleanup work.
tty core just restore to the tty old ldisc.
That always causes memory leak.
Nice catch!
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2382,7 +2383,13 @@ static int gsmld_open(struct tty_struct *tty)
/* Attach the initial passive connection */
gsm-encoding = 1;
-return gsmld_attach_gsm(tty, gsm);
+
+ret = gsmld_attach_gsm(tty, gsm);
+if (ret != 0) {
+gsm_cleanup_mux(gsm);
+mux_put(gsm);
It is quite illogical to put the mux here. It should be in gsmld_open.
I.e. gsm_cleanup_mux here, mux_put there.
Thanks for your reply :)
But I am a little confused with your comments, could you explain it when you
are free?
Sorry for my poor English.
thanks,
xinhui
thanks,
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty/n_gsm.c: fix a memory leak in gsmld_open
于 2014年07月28日 17:03, xinhui.pan 写道:
Hi, Jiri
于 2014年07月28日 16:49, Jiri Slaby 写道:
On 07/28/2014 10:14 AM, xinhui.pan wrote:
If gsmld_attach_gsm fails, the gsm is not used anymore.
tty core will not call gsmld_close to do the cleanup work.
tty core just restore to the tty old ldisc.
That always causes memory leak.
Nice catch!
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2382,7 +2383,13 @@ static int gsmld_open(struct tty_struct *tty)
/* Attach the initial passive connection */
gsm-encoding = 1;
- return gsmld_attach_gsm(tty, gsm);
+
+ ret = gsmld_attach_gsm(tty, gsm);
+ if (ret != 0) {
+ gsm_cleanup_mux(gsm);
+ mux_put(gsm);
It is quite illogical to put the mux here. It should be in gsmld_open.
I.e. gsm_cleanup_mux here, mux_put there.
Thanks for your reply :)
But I am a little confused with your comments, could you explain it when you
are free?
Sorry for my poor English.
hi, Jiri
I guess you want gsm_cleanup_mux() called in gsmld_attach_gsm(), just
after gsm_activate_mux() fails?
Yes, that seems really make sence. :)
Thanks for pointing out it.
Let me explain my opinion. :)
Actually gsmld_attach_gsm results in two different effects when it fails.
(a)gsmld_attach_gsm - gsm_activate_mux(gsm_mux[] is full,-EBUSY), then
gsm_mux[] did not change, and gsm-num is invaild;
(b)gsmld_attach_gsm - gsm_activate_mux(return -ENOMEM), then gsm_mux[]
changes, and gsm-num is vaild.
To be honest, I am not very clear about this. I even suspect this is done in
such way on purpose.
So I just keep the code what it is now. Let's handle the error in gsmld_open().
We can make sure that gsm is not used here and it's safe to cleanup and put it.
thanks,
xinhui
thanks,
xinhui
thanks,
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty/tty_io.c: make a check before reuse cdev
于 2014年07月24日 00:07, One Thousand Gnomes 写道: >> Very nice solution. We will check if this can cause any risk, both to kernel >> and user space. >> Using a new tty base to register with new cdevs may give us more chance to >> wait PROCESS quit/close. >> when total 256 tty used up, what we should do is still in discuss. > > At that point you may want to look at how fuser works and create some > kind of policy manager needs to kill problem tasks owning a device. > > Or in theory there is no reason nowdays we can't go above 256 devices - > in theory 8) > hi, Alan Thanks for your nice comments. In theory it can go over 256 devices, In fact, it will waste more memories. :) We really need a policy manager that works well. FT guys may offer it one day. it's a little long time maybe. :( When I was a student, I heard you left from kernel. Welcome back to kernel, I didn't know that you are back. So sorry. thanks, xinhui > Alan > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty/tty_io.c: make a check before reuse cdev
于 2014年07月24日 00:07, One Thousand Gnomes 写道: >> Very nice solution. We will check if this can cause any risk, both to kernel >> and user space. >> Using a new tty base to register with new cdevs may give us more chance to >> wait PROCESS quit/close. >> when total 256 tty used up, what we should do is still in discuss. > > At that point you may want to look at how fuser works and create some > kind of policy manager needs to kill problem tasks owning a device. > > Or in theory there is no reason nowdays we can't go above 256 devices - > in theory 8) > hi, Alan Thanks for your nice comments. In theory it can go over 256 devices, In fact, it will waste more memories. :) We really need a policy manager that works well. FT guys may offer it one day. it's a little long time maybe. :( When I was a student, I heard you leave from kernel. Welcome back to kernel, I didn't know that you are back. So sorry. > Alan > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty/tty_io.c: make a check before reuse cdev
hi, Peter 于 2014年07月24日 00:04, Peter Hurley 写道: > Hi Xinhui, > > On 07/23/2014 05:21 AM, xinhui.pan wrote: >> 于 2014年07月23日 00:40, Peter Hurley 写道: >>> On 07/22/2014 07:52 AM, xinhui.pan wrote: >>>> 于 2014年07月21日 23:38, Greg KH 写道: >>>>> On Mon, Jul 21, 2014 at 08:47:16PM +0800, pp wrote: > >>>>>> tty driver register its device and (D)init the cdevs again. >>>>> >>>>> What driver does this with an "old" device, it should have created a new >>>>> one, otherwise, as you have pointed out, it's a bug. >>>>> >>>> >>>> I can't agree more with you. we should not use "old" device. >>> >>> This is a gsm driver problem. The GSM driver is reusing device indexes >>> for still-open ttys. >>> >>> The GSM driver uses a global table, gsm_mux[], to allocate device indexes >>> but prematurely clears the table entry in gsm_mux_cleanup(). If instead, >>> clearing the gsm_mux table entry were deferred to gsm_mux_free(), then >>> device indexes would not be getting reused until after the last tty >>> associated with the last gsm attach was closed. >>> >> >> Very nice solution. We will check if this can cause any risk, both to kernel >> and user space. >> Using a new tty base to register with new cdevs may give us more chance to >> wait PROCESS quit/close. >> when total 256 tty used up, what we should do is still in discuss. > > I saw your patch for the use of gsm->num before gsm_activate_mux() has > allocated the table entry; thanks for fixing that. > > As for what to do if all the gsm_mux table entries are used: if the error > is infrequent, I suggest simply returning an error which is what the > driver does currently. Otherwise, a more dynamic allocation scheme may be > required. > Yes, agree with you. This error is infrequent. Intel software will close the gsmtty after read/write hit errors. We don't need dynamic allocation. So keep returning an error here. :) > I did notice while reviewing the error handling that gsmld_open() will > leak the entire composite ldisc data allocated by gsm_alloc_mux() if > gsmld_attach_gsm() fails. > Thanks for your reviewing. Do you mean gsm = gsm_alloc_mux() will cause leak if gsmld_attach_gsm fails? As there is no gsm_free_mux()? If so, Yes, it is. In such scenario, gsmld_close is not called. and gsm_free_mux is not called, either. Thanks for your nice comments. You really help us fix several ugly issues. Let me have a deep think about it. thanks, xinhui > Regards, > Peter Hurley > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] tty/n_gsm.c: do not clear gsm_mux entry when the gsm is not closed
If the gsmtty is still used by some process, we could not just
simply clear gsm_mux[gsm->num]. Clear it when gsm is being free.
Otherwise we will hit crashes when userspace close the gsmtty.
Also add gsm_mux_get() and gsm_mux_put() to make gsm_mux[] is used safely.
We can do activation/deactivation with same gsm more than once now.
This is for fixing the FIXME.
Signed-off-by: xinhui.pan
Reviewed-by: Zhang Yanmin
---
drivers/tty/n_gsm.c | 84 ---
1 file changed, 60 insertions(+), 24 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 81e7ccb..290df56 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2020,6 +2020,58 @@ static void gsm_error(struct gsm_mux *gsm,
}
/**
+ * gsm_mux_get - get/fill one entry in gsm_mux
+ * @gsm: our gsm
+ *
+ * Although its name end with get, it don't inc ref-count actually.
+ * get one entry is just like fill pte, first memory access will
+ * cause page_fault, the next accesses don't. So do here.
+ */
+
+static int gsm_mux_get(struct gsm_mux *gsm)
+{
+ int i;
+
+ if (gsm->num >= MAX_MUX) /* gsm is alloc by kzalloc, just be careful */
+ return -EIO;
+ if (gsm_mux[gsm->num] == gsm) /* We have already set gsm->num */
+ return 0;
+
+ spin_lock(_mux_lock);
+ for (i = 0; i < MAX_MUX; i++) {
+ if (gsm_mux[i] == NULL) {
+ gsm->num = i;
+ gsm_mux[i] = gsm;
+ break;
+ }
+ }
+ spin_unlock(_mux_lock);
+
+ if (i == MAX_MUX)
+ return -EBUSY;
+ return 0;
+}
+
+/**
+ * gsm_mux_put - put/clear one entry in gsm_mux
+ * @gsm: our gsm
+ *
+ * Although its name end with put, it don't dec ref-count actually.
+ * put one entry is just like clear pte, So do here.
+ */
+
+static void gsm_mux_put(struct gsm_mux *gsm)
+{
+ if (gsm->num >= MAX_MUX)
+ return;
+
+ spin_lock(_mux_lock);
+ if (gsm_mux[gsm->num] == gsm)
+ gsm_mux[gsm->num] = NULL;
+ spin_unlock(_mux_lock);
+}
+
+/**
* gsm_cleanup_mux - generic GSM protocol cleanup
* @gsm: our mux
*
@@ -2037,16 +2089,6 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm)
gsm->dead = 1;
- spin_lock(_mux_lock);
- for (i = 0; i < MAX_MUX; i++) {
- if (gsm_mux[i] == gsm) {
- gsm_mux[i] = NULL;
- break;
- }
- }
- spin_unlock(_mux_lock);
- WARN_ON(i == MAX_MUX);
-
/* In theory disconnecting DLCI 0 is sufficient but for some
modems this is apparently not the case. */
if (dlci) {
@@ -2086,7 +2128,7 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm)
static int gsm_activate_mux(struct gsm_mux *gsm)
{
struct gsm_dlci *dlci;
- int i = 0;
+ int ret = 0;
init_timer(>t2_timer);
gsm->t2_timer.function = gsm_control_retransmit;
@@ -2101,17 +2143,12 @@ static int gsm_activate_mux(struct gsm_mux *gsm)
gsm->receive = gsm1_receive;
gsm->error = gsm_error;
- spin_lock(_mux_lock);
- for (i = 0; i < MAX_MUX; i++) {
- if (gsm_mux[i] == NULL) {
- gsm->num = i;
- gsm_mux[i] = gsm;
- break;
- }
- }
- spin_unlock(_mux_lock);
- if (i == MAX_MUX)
- return -EBUSY;
+ /*
+ * call gsm_mux_get more than once is safe with same gsm
+ */
+ ret = gsm_mux_get(gsm);
+ if (ret)
+ return ret;
dlci = gsm_dlci_alloc(gsm, 0);
if (dlci == NULL)
@@ -2142,6 +2179,7 @@ static void gsm_free_mux(struct gsm_mux *gsm)
static void gsm_free_muxr(struct kref *ref)
{
struct gsm_mux *gsm = container_of(ref, struct gsm_mux, ref);
+ gsm_mux_put(gsm);
gsm_free_mux(gsm);
}
@@ -2559,8 +2597,6 @@ static int gsmld_config(struct tty_struct *tty, struct
gsm_mux *gsm,
if (c->t2)
gsm->t2 = c->t2;
- /* FIXME: We need to separate activation/deactivation from adding
- and removing from the mux array */
if (need_restart)
gsm_activate_mux(gsm);
if (gsm->initiator && need_close)
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] tty/n_gsm.c: do not clear gsm_mux entry when the gsm is not closed
If the gsmtty is still used by some process, we could not just
simply clear gsm_mux[gsm-num]. Clear it when gsm is being free.
Otherwise we will hit crashes when userspace close the gsmtty.
Also add gsm_mux_get() and gsm_mux_put() to make gsm_mux[] is used safely.
We can do activation/deactivation with same gsm more than once now.
This is for fixing the FIXME.
Signed-off-by: xinhui.pan xinhuix@intel.com
Reviewed-by: Zhang Yanmin yanmin_zh...@linux.intel.com
---
drivers/tty/n_gsm.c | 84 ---
1 file changed, 60 insertions(+), 24 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 81e7ccb..290df56 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2020,6 +2020,58 @@ static void gsm_error(struct gsm_mux *gsm,
}
/**
+ * gsm_mux_get - get/fill one entry in gsm_mux
+ * @gsm: our gsm
+ *
+ * Although its name end with get, it don't inc ref-count actually.
+ * get one entry is just like fill pte, first memory access will
+ * cause page_fault, the next accesses don't. So do here.
+ */
+
+static int gsm_mux_get(struct gsm_mux *gsm)
+{
+ int i;
+
+ if (gsm-num = MAX_MUX) /* gsm is alloc by kzalloc, just be careful */
+ return -EIO;
+ if (gsm_mux[gsm-num] == gsm) /* We have already set gsm-num */
+ return 0;
+
+ spin_lock(gsm_mux_lock);
+ for (i = 0; i MAX_MUX; i++) {
+ if (gsm_mux[i] == NULL) {
+ gsm-num = i;
+ gsm_mux[i] = gsm;
+ break;
+ }
+ }
+ spin_unlock(gsm_mux_lock);
+
+ if (i == MAX_MUX)
+ return -EBUSY;
+ return 0;
+}
+
+/**
+ * gsm_mux_put - put/clear one entry in gsm_mux
+ * @gsm: our gsm
+ *
+ * Although its name end with put, it don't dec ref-count actually.
+ * put one entry is just like clear pte, So do here.
+ */
+
+static void gsm_mux_put(struct gsm_mux *gsm)
+{
+ if (gsm-num = MAX_MUX)
+ return;
+
+ spin_lock(gsm_mux_lock);
+ if (gsm_mux[gsm-num] == gsm)
+ gsm_mux[gsm-num] = NULL;
+ spin_unlock(gsm_mux_lock);
+}
+
+/**
* gsm_cleanup_mux - generic GSM protocol cleanup
* @gsm: our mux
*
@@ -2037,16 +2089,6 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm)
gsm-dead = 1;
- spin_lock(gsm_mux_lock);
- for (i = 0; i MAX_MUX; i++) {
- if (gsm_mux[i] == gsm) {
- gsm_mux[i] = NULL;
- break;
- }
- }
- spin_unlock(gsm_mux_lock);
- WARN_ON(i == MAX_MUX);
-
/* In theory disconnecting DLCI 0 is sufficient but for some
modems this is apparently not the case. */
if (dlci) {
@@ -2086,7 +2128,7 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm)
static int gsm_activate_mux(struct gsm_mux *gsm)
{
struct gsm_dlci *dlci;
- int i = 0;
+ int ret = 0;
init_timer(gsm-t2_timer);
gsm-t2_timer.function = gsm_control_retransmit;
@@ -2101,17 +2143,12 @@ static int gsm_activate_mux(struct gsm_mux *gsm)
gsm-receive = gsm1_receive;
gsm-error = gsm_error;
- spin_lock(gsm_mux_lock);
- for (i = 0; i MAX_MUX; i++) {
- if (gsm_mux[i] == NULL) {
- gsm-num = i;
- gsm_mux[i] = gsm;
- break;
- }
- }
- spin_unlock(gsm_mux_lock);
- if (i == MAX_MUX)
- return -EBUSY;
+ /*
+ * call gsm_mux_get more than once is safe with same gsm
+ */
+ ret = gsm_mux_get(gsm);
+ if (ret)
+ return ret;
dlci = gsm_dlci_alloc(gsm, 0);
if (dlci == NULL)
@@ -2142,6 +2179,7 @@ static void gsm_free_mux(struct gsm_mux *gsm)
static void gsm_free_muxr(struct kref *ref)
{
struct gsm_mux *gsm = container_of(ref, struct gsm_mux, ref);
+ gsm_mux_put(gsm);
gsm_free_mux(gsm);
}
@@ -2559,8 +2597,6 @@ static int gsmld_config(struct tty_struct *tty, struct
gsm_mux *gsm,
if (c-t2)
gsm-t2 = c-t2;
- /* FIXME: We need to separate activation/deactivation from adding
- and removing from the mux array */
if (need_restart)
gsm_activate_mux(gsm);
if (gsm-initiator need_close)
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty/tty_io.c: make a check before reuse cdev
hi, Peter 于 2014年07月24日 00:04, Peter Hurley 写道: Hi Xinhui, On 07/23/2014 05:21 AM, xinhui.pan wrote: 于 2014年07月23日 00:40, Peter Hurley 写道: On 07/22/2014 07:52 AM, xinhui.pan wrote: 于 2014年07月21日 23:38, Greg KH 写道: On Mon, Jul 21, 2014 at 08:47:16PM +0800, pp wrote: tty driver register its device and (D)init the cdevs again. What driver does this with an old device, it should have created a new one, otherwise, as you have pointed out, it's a bug. I can't agree more with you. we should not use old device. This is a gsm driver problem. The GSM driver is reusing device indexes for still-open ttys. The GSM driver uses a global table, gsm_mux[], to allocate device indexes but prematurely clears the table entry in gsm_mux_cleanup(). If instead, clearing the gsm_mux table entry were deferred to gsm_mux_free(), then device indexes would not be getting reused until after the last tty associated with the last gsm attach was closed. Very nice solution. We will check if this can cause any risk, both to kernel and user space. Using a new tty base to register with new cdevs may give us more chance to wait PROCESS quit/close. when total 256 tty used up, what we should do is still in discuss. I saw your patch for the use of gsm-num before gsm_activate_mux() has allocated the table entry; thanks for fixing that. As for what to do if all the gsm_mux table entries are used: if the error is infrequent, I suggest simply returning an error which is what the driver does currently. Otherwise, a more dynamic allocation scheme may be required. Yes, agree with you. This error is infrequent. Intel software will close the gsmtty after read/write hit errors. We don't need dynamic allocation. So keep returning an error here. :) I did notice while reviewing the error handling that gsmld_open() will leak the entire composite ldisc data allocated by gsm_alloc_mux() if gsmld_attach_gsm() fails. Thanks for your reviewing. Do you mean gsm = gsm_alloc_mux() will cause leak if gsmld_attach_gsm fails? As there is no gsm_free_mux()? If so, Yes, it is. In such scenario, gsmld_close is not called. and gsm_free_mux is not called, either. Thanks for your nice comments. You really help us fix several ugly issues. Let me have a deep think about it. thanks, xinhui Regards, Peter Hurley -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty/tty_io.c: make a check before reuse cdev
于 2014年07月24日 00:07, One Thousand Gnomes 写道: Very nice solution. We will check if this can cause any risk, both to kernel and user space. Using a new tty base to register with new cdevs may give us more chance to wait PROCESS quit/close. when total 256 tty used up, what we should do is still in discuss. At that point you may want to look at how fuser works and create some kind of policy manager needs to kill problem tasks owning a device. Or in theory there is no reason nowdays we can't go above 256 devices - in theory 8) hi, Alan Thanks for your nice comments. In theory it can go over 256 devices, In fact, it will waste more memories. :) We really need a policy manager that works well. FT guys may offer it one day. it's a little long time maybe. :( When I was a student, I heard you leave from kernel. Welcome back to kernel, I didn't know that you are back. So sorry. Alan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty/tty_io.c: make a check before reuse cdev
于 2014年07月24日 00:07, One Thousand Gnomes 写道: Very nice solution. We will check if this can cause any risk, both to kernel and user space. Using a new tty base to register with new cdevs may give us more chance to wait PROCESS quit/close. when total 256 tty used up, what we should do is still in discuss. At that point you may want to look at how fuser works and create some kind of policy manager needs to kill problem tasks owning a device. Or in theory there is no reason nowdays we can't go above 256 devices - in theory 8) hi, Alan Thanks for your nice comments. In theory it can go over 256 devices, In fact, it will waste more memories. :) We really need a policy manager that works well. FT guys may offer it one day. it's a little long time maybe. :( When I was a student, I heard you left from kernel. Welcome back to kernel, I didn't know that you are back. So sorry. thanks, xinhui Alan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty/tty_io.c: make a check before reuse cdev
于 2014年07月23日 00:40, Peter Hurley 写道: > On 07/22/2014 07:52 AM, xinhui.pan wrote: >> >> 于 2014年07月21日 23:38, Greg KH 写道: >>> On Mon, Jul 21, 2014 at 08:47:16PM +0800, pp wrote: >>>> As reuse the cdev may cause panic. After we unregister the tty device, we >>>> may use tty_hangup() o >>>> other similar function to send a signal(SIGHUP) to process which has opend >>>> our device. But that >>>> not succeed if the process couldn't get the signal. for example, a process >>>> forked >>>> but his parent quited never get SIGHUP. >>>> >>>> Here is our scence. >>>> tty driver register its device and init the cdevs, then process "A" open >>>> one cdev. >>>> tty driver unregister its device and cdev_del the cdevs, call tty_hangup >>>> to (S)send signal SIGHUP to process A. >>>> But that step(S) fails. >>> >>> How can that fail? What driver does this fail for? >> >> hi, Greg >> Thanks for your nice comments. :) >> It's gsm driver that want to unregister/register tty device. We are >> working on our intel mobile phone, >> When the phone goes into airplane-mode, the modem will disconnect from >> system, then gsmld_close() -> gsmld_detach_gsm() -> tty_unregister_device(). >> When the phone leaves airplane-mode, the modem will connect to system, then >> gsmld_open() -> gsmld_attach_gsm() -> tty_register_device() >> In this way how gsm driver works. >> It seems very normal and can work well. :) >> >> But there is always something bad for us to deal with. >> If a process(A, its name) opens the /dev/gsmttyXX, and the process(A) is, >> for example, running with command "A &". >> The process(A) is not able to receive the signal SIGHUP from __tty_hangup() >> -> tty_signal_session_leader(). >> There are several reasons that can stop process(A) from receiving signal >> SIGHUP. >> another example, B is running, and he makes a fork(), A is the child of B, >> then B quit, leave A running. >> in such scenario, A is not able to receive signal SIGHUP, either. >> Anyway, we cannot guarantee process(A) will close /dev/gsmttyXX in time. >> That means we don't know when we can reuse the tty_driver->cdevs[XX]. >> one second, one minute? We don't know. We just don't trust user space. :) > > Or a process could simply ignore SIGHUP, in which case /dev/gsmttyXX > will not be closed until process termination. > hi, Peter Agree with you. Thanks for your nice comments. >>>> tty driver register its device and (D)init the cdevs again. >>> >>> What driver does this with an "old" device, it should have created a new >>> one, otherwise, as you have pointed out, it's a bug. >>> >> >> I can't agree more with you. we should not use "old" device. > > This is a gsm driver problem. The GSM driver is reusing device indexes > for still-open ttys. > > The GSM driver uses a global table, gsm_mux[], to allocate device indexes > but prematurely clears the table entry in gsm_mux_cleanup(). If instead, > clearing the gsm_mux table entry were deferred to gsm_mux_free(), then > device indexes would not be getting reused until after the last tty > associated with the last gsm attach was closed. > Very nice solution. We will check if this can cause any risk, both to kernel and user space. Using a new tty base to register with new cdevs may give us more chance to wait PROCESS quit/close. when total 256 tty used up, what we should do is still in discuss. thanks, I even want to have a cup of coffee with you :) thanks xinhui > Regards, > Peter Hurley > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] tty/n_gsm.c: get gsm->num after gsm_activate_mux
gsm->num is the index of gsm_mux[], it's invalid before calling
gsm_activate_mux.
Signed-off-by: xinhui.pan
---
drivers/tty/n_gsm.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 2ebe47b..81e7ccb 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2230,8 +2230,7 @@ static int gsmld_output(struct gsm_mux *gsm, u8 *data,
int len)
static int gsmld_attach_gsm(struct tty_struct *tty, struct gsm_mux *gsm)
{
- int ret, i;
- int base = gsm->num << 6; /* Base for this MUX */
+ int ret, i, base;
gsm->tty = tty_kref_get(tty);
gsm->output = gsmld_output;
@@ -2241,6 +2240,7 @@ static int gsmld_attach_gsm(struct tty_struct *tty,
struct gsm_mux *gsm)
else {
/* Don't register device 0 - this is the control channel and not
a usable tty interface */
+ base = gsm->num << 6; /* Base for this MUX */
for (i = 1; i < NUM_DLCI; i++)
tty_register_device(gsm_tty_driver, base + i, NULL);
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] tty/n_gsm.c: get gsm-num after gsm_activate_mux
gsm-num is the index of gsm_mux[], it's invalid before calling
gsm_activate_mux.
Signed-off-by: xinhui.pan xinhuix@intel.com
---
drivers/tty/n_gsm.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 2ebe47b..81e7ccb 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2230,8 +2230,7 @@ static int gsmld_output(struct gsm_mux *gsm, u8 *data,
int len)
static int gsmld_attach_gsm(struct tty_struct *tty, struct gsm_mux *gsm)
{
- int ret, i;
- int base = gsm-num 6; /* Base for this MUX */
+ int ret, i, base;
gsm-tty = tty_kref_get(tty);
gsm-output = gsmld_output;
@@ -2241,6 +2240,7 @@ static int gsmld_attach_gsm(struct tty_struct *tty,
struct gsm_mux *gsm)
else {
/* Don't register device 0 - this is the control channel and not
a usable tty interface */
+ base = gsm-num 6; /* Base for this MUX */
for (i = 1; i NUM_DLCI; i++)
tty_register_device(gsm_tty_driver, base + i, NULL);
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty/tty_io.c: make a check before reuse cdev
于 2014年07月23日 00:40, Peter Hurley 写道: On 07/22/2014 07:52 AM, xinhui.pan wrote: 于 2014年07月21日 23:38, Greg KH 写道: On Mon, Jul 21, 2014 at 08:47:16PM +0800, pp wrote: As reuse the cdev may cause panic. After we unregister the tty device, we may use tty_hangup() o other similar function to send a signal(SIGHUP) to process which has opend our device. But that not succeed if the process couldn't get the signal. for example, a process forked but his parent quited never get SIGHUP. Here is our scence. tty driver register its device and init the cdevs, then process A open one cdev. tty driver unregister its device and cdev_del the cdevs, call tty_hangup to (S)send signal SIGHUP to process A. But that step(S) fails. How can that fail? What driver does this fail for? hi, Greg Thanks for your nice comments. :) It's gsm driver that want to unregister/register tty device. We are working on our intel mobile phone, When the phone goes into airplane-mode, the modem will disconnect from system, then gsmld_close() - gsmld_detach_gsm() - tty_unregister_device(). When the phone leaves airplane-mode, the modem will connect to system, then gsmld_open() - gsmld_attach_gsm() - tty_register_device() In this way how gsm driver works. It seems very normal and can work well. :) But there is always something bad for us to deal with. If a process(A, its name) opens the /dev/gsmttyXX, and the process(A) is, for example, running with command A . The process(A) is not able to receive the signal SIGHUP from __tty_hangup() - tty_signal_session_leader(). There are several reasons that can stop process(A) from receiving signal SIGHUP. another example, B is running, and he makes a fork(), A is the child of B, then B quit, leave A running. in such scenario, A is not able to receive signal SIGHUP, either. Anyway, we cannot guarantee process(A) will close /dev/gsmttyXX in time. That means we don't know when we can reuse the tty_driver-cdevs[XX]. one second, one minute? We don't know. We just don't trust user space. :) Or a process could simply ignore SIGHUP, in which case /dev/gsmttyXX will not be closed until process termination. hi, Peter Agree with you. Thanks for your nice comments. tty driver register its device and (D)init the cdevs again. What driver does this with an old device, it should have created a new one, otherwise, as you have pointed out, it's a bug. I can't agree more with you. we should not use old device. This is a gsm driver problem. The GSM driver is reusing device indexes for still-open ttys. The GSM driver uses a global table, gsm_mux[], to allocate device indexes but prematurely clears the table entry in gsm_mux_cleanup(). If instead, clearing the gsm_mux table entry were deferred to gsm_mux_free(), then device indexes would not be getting reused until after the last tty associated with the last gsm attach was closed. Very nice solution. We will check if this can cause any risk, both to kernel and user space. Using a new tty base to register with new cdevs may give us more chance to wait PROCESS quit/close. when total 256 tty used up, what we should do is still in discuss. thanks, I even want to have a cup of coffee with you :) thanks xinhui Regards, Peter Hurley -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty/tty_io.c: make a check before reuse cdev
于 2014年07月21日 23:38, Greg KH 写道: > On Mon, Jul 21, 2014 at 08:47:16PM +0800, pp wrote: >> As reuse the cdev may cause panic. After we unregister the tty device, we >> may use tty_hangup() o >> other similar function to send a signal(SIGHUP) to process which has opend >> our device. But that >> not succeed if the process couldn't get the signal. for example, a process >> forked >> but his parent quited never get SIGHUP. >> >> Here is our scence. >> tty driver register its device and init the cdevs, then process "A" open one >> cdev. >> tty driver unregister its device and cdev_del the cdevs, call tty_hangup to >> (S)send signal SIGHUP to process A. >> But that step(S) fails. > > How can that fail? What driver does this fail for? hi, Greg Thanks for your nice comments. :) It's gsm driver that want to unregister/register tty device. We are working on our intel mobile phone, When the phone goes into airplane-mode, the modem will disconnect from system, then gsmld_close() -> gsmld_detach_gsm() -> tty_unregister_device(). When the phone leaves airplane-mode, the modem will connect to system, then gsmld_open() -> gsmld_attach_gsm() -> tty_register_device() In this way how gsm driver works. It seems very normal and can work well. :) But there is always something bad for us to deal with. If a process(A, its name) opens the /dev/gsmttyXX, and the process(A) is, for example, running with command "A &". The process(A) is not able to receive the signal SIGHUP from __tty_hangup() -> tty_signal_session_leader(). There are several reasons that can stop process(A) from receiving signal SIGHUP. another example, B is running, and he makes a fork(), A is the child of B, then B quit, leave A running. in such scenario, A is not able to receive signal SIGHUP, either. Anyway, we cannot guarantee process(A) will close /dev/gsmttyXX in time. That means we don't know when we can reuse the tty_driver->cdevs[XX]. one second, one minute? We don't know. We just don't trust user space. :) > >> tty driver register its device and (D)init the cdevs again. > > What driver does this with an "old" device, it should have created a new > one, otherwise, as you have pointed out, it's a bug. > I can't agree more with you. we should not use "old" device. gsm driver(gsm_tty_driver) call tty_register_device() to export gsmttyXX to userspace. but tty core code manages cdevs by itself. cdevs is not directly used by any other tty drivers. Adding a check in n_gsm.c before call tty_register_device() seems breaking a rule and making things complex in my opinion. I also find a fact that most tty drivers only call tty_register_device() in probe(), and call tty_unregister_device() in remove(). Q1: Is this a rule, too? :) I think my patch is not good enough. Q2: How about (1)change struct *cdevs to struct **cdevs, and (2)use cdev_alloc() instead of cdev_init()? :) Let char_dev core code manage the cdev. when the cdev's ref-count become zero, cdev will be released by char_dev core code self. I very appreciate for your comments. and your advices are very important and helpful for me to solve this issue. thanks, xinhui > thanks, > > greg k-h > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty/tty_io.c: make a check before reuse cdev
于 2014年07月21日 23:38, Greg KH 写道: On Mon, Jul 21, 2014 at 08:47:16PM +0800, pp wrote: As reuse the cdev may cause panic. After we unregister the tty device, we may use tty_hangup() o other similar function to send a signal(SIGHUP) to process which has opend our device. But that not succeed if the process couldn't get the signal. for example, a process forked but his parent quited never get SIGHUP. Here is our scence. tty driver register its device and init the cdevs, then process A open one cdev. tty driver unregister its device and cdev_del the cdevs, call tty_hangup to (S)send signal SIGHUP to process A. But that step(S) fails. How can that fail? What driver does this fail for? hi, Greg Thanks for your nice comments. :) It's gsm driver that want to unregister/register tty device. We are working on our intel mobile phone, When the phone goes into airplane-mode, the modem will disconnect from system, then gsmld_close() - gsmld_detach_gsm() - tty_unregister_device(). When the phone leaves airplane-mode, the modem will connect to system, then gsmld_open() - gsmld_attach_gsm() - tty_register_device() In this way how gsm driver works. It seems very normal and can work well. :) But there is always something bad for us to deal with. If a process(A, its name) opens the /dev/gsmttyXX, and the process(A) is, for example, running with command A . The process(A) is not able to receive the signal SIGHUP from __tty_hangup() - tty_signal_session_leader(). There are several reasons that can stop process(A) from receiving signal SIGHUP. another example, B is running, and he makes a fork(), A is the child of B, then B quit, leave A running. in such scenario, A is not able to receive signal SIGHUP, either. Anyway, we cannot guarantee process(A) will close /dev/gsmttyXX in time. That means we don't know when we can reuse the tty_driver-cdevs[XX]. one second, one minute? We don't know. We just don't trust user space. :) tty driver register its device and (D)init the cdevs again. What driver does this with an old device, it should have created a new one, otherwise, as you have pointed out, it's a bug. I can't agree more with you. we should not use old device. gsm driver(gsm_tty_driver) call tty_register_device() to export gsmttyXX to userspace. but tty core code manages cdevs by itself. cdevs is not directly used by any other tty drivers. Adding a check in n_gsm.c before call tty_register_device() seems breaking a rule and making things complex in my opinion. I also find a fact that most tty drivers only call tty_register_device() in probe(), and call tty_unregister_device() in remove(). Q1: Is this a rule, too? :) I think my patch is not good enough. Q2: How about (1)change struct *cdevs to struct **cdevs, and (2)use cdev_alloc() instead of cdev_init()? :) Let char_dev core code manage the cdev. when the cdev's ref-count become zero, cdev will be released by char_dev core code self. I very appreciate for your comments. and your advices are very important and helpful for me to solve this issue. thanks, xinhui thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH] md/dm-ioctl.c: optimize memory allocation in copy_params
于 2014年07月09日 10:01, Zhang, Yanmin 写道:
> On 2014/7/9 6:39, Mikulas Patocka wrote:
>
>> Hi
>
> Mikulas,
>
> Thanks for your kind comments.
>
>> I don't really know what is the purpose of this patch. In existing device
>> mapper code, if kmalloc fails, the allocation is retried with __vmalloc.
>> So there is no need to avoid kmalloc aritifically.
>>
>> kmalloc doesn't cause memory fragmentation. If the memory is too
>> fragmented, kmalloc fails. If it isn't, it succeeds. But it doesn't cause
>> memory being fragmented.
>
> I agree with you. The patch's original description is not appropriate.
> Basically, memory fragmentation is not caused by this kmalloc.
>
> The right description is: When memory is fragmented and most memory is used
> up,
> kmalloc a big memory might cause lots of OutOFMemory and system might kill
> lots of processes. Then, system might hang.
>
> We are enabling Android on mobiles and tablets. We hit OOM often at
> Monkey and other stress testing. Sometimes, kernel prints big memory
> allocation
> warning.
>
> Theoretically, it's a good idea to call kmalloc firstly. If it fails, use
> vmalloc.
> However, kernel assumes drivers do the right thing. When drivers call kmalloc
> to
> allocate a big memory, memory management would do the best to fulfill it.
> When memory
> is fragmented and most memory is used up, such allocation would cause big
> memory
> recollection. Some processes might be killed. The worse scenario is after a
> process
> is killed, it is restarted and allocates memory again. That causes system
> hang,
> or mobile users feel a big stall. We did fix a similar issue in one of our
> drivers.
>
> Usually, kernel and drivers can allocate big continuous memory at booting or
> initialization stage. After that, they need allocate small order memory. The
> best
> is to just allocate order-0 memory.
>
>
>>
>> If you have some other driver that fails because of large kmalloc failure,
>> you should fix it to use scatter-gather DMA or (if the hardware can't do
>> it) preallocate the memory when the driver is loaded.
>
> I agree with you. Here the patch fixes the issue, where dm might allocate
> big continuous memory after booting. Monkey testing triggers it by operating
> menu Setting=>Security=>InstallfromSDcard.
>
hi, Yanmin && Mikulas
Thanks for your nice comments. And sorry for confusing you as my
comment is not clear enough.
In android, there is a command "dumpstate", it run many other commands to
collect information. In our scenario,
it run command "vdc dump", and vdc uses socket to pass some parameters to
"vold", then vold generates ioctl.
thanks.
> We are catching all places in kernel where big memory allocation happens.
> This patch is just to fix one case.
>
> Thanks,
> Yanmin
>
>>
>> Mikulas
>>
>>
>>
>> On Fri, 4 Jul 2014, xinhui.pan wrote:
>>
>>> KMALLOC_MAX_SIZE is too big, it easily cause memory fragmented and low
>>> memory when param_kernel->data_size
>>> is also big. That's not nice. So use vmalloc instead of kmalloc when size
>>> is larger than (PAGE_SIZE << 2).
>>> There are other drivers using kmalloc to gain a big size memory. And that
>>> cause our devices to hit hang and
>>> many worse issues. We don't benefit much when using kmalloc in such
>>> scenario.
>>>
>>> Signed-off-by: xinhui.pan
>>> Signed-off-by: yanmin.zhang
>>> ---
>>> drivers/md/dm-ioctl.c |2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
>>> index 5152142..31c3af9 100644
>>> --- a/drivers/md/dm-ioctl.c
>>> +++ b/drivers/md/dm-ioctl.c
>>> @@ -1709,7 +1709,7 @@ static int copy_params(struct dm_ioctl __user *user,
>>> struct dm_ioctl *param_kern
>>> * Use kmalloc() rather than vmalloc() when we can.
>>> */
>>> dmi = NULL;
>>> - if (param_kernel->data_size <= KMALLOC_MAX_SIZE) {
>>> + if (param_kernel->data_size <= (PAGE_SIZE << 2)) {
>>> dmi = kmalloc(param_kernel->data_size, GFP_NOIO |
>>> __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN);
>>> if (dmi)
>>> *param_flags |= DM_PARAMS_KMALLOC;
>>> --
>>> 1.7.9.5
>>>
>>> --
>>> dm-devel mailing list
>>> dm-de...@redhat.com
>>> https://www.redhat.com/mailman/listinfo/dm-devel
>>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH] md/dm-ioctl.c: optimize memory allocation in copy_params
于 2014年07月09日 10:01, Zhang, Yanmin 写道:
On 2014/7/9 6:39, Mikulas Patocka wrote:
Hi
Mikulas,
Thanks for your kind comments.
I don't really know what is the purpose of this patch. In existing device
mapper code, if kmalloc fails, the allocation is retried with __vmalloc.
So there is no need to avoid kmalloc aritifically.
kmalloc doesn't cause memory fragmentation. If the memory is too
fragmented, kmalloc fails. If it isn't, it succeeds. But it doesn't cause
memory being fragmented.
I agree with you. The patch's original description is not appropriate.
Basically, memory fragmentation is not caused by this kmalloc.
The right description is: When memory is fragmented and most memory is used
up,
kmalloc a big memory might cause lots of OutOFMemory and system might kill
lots of processes. Then, system might hang.
We are enabling Android on mobiles and tablets. We hit OOM often at
Monkey and other stress testing. Sometimes, kernel prints big memory
allocation
warning.
Theoretically, it's a good idea to call kmalloc firstly. If it fails, use
vmalloc.
However, kernel assumes drivers do the right thing. When drivers call kmalloc
to
allocate a big memory, memory management would do the best to fulfill it.
When memory
is fragmented and most memory is used up, such allocation would cause big
memory
recollection. Some processes might be killed. The worse scenario is after a
process
is killed, it is restarted and allocates memory again. That causes system
hang,
or mobile users feel a big stall. We did fix a similar issue in one of our
drivers.
Usually, kernel and drivers can allocate big continuous memory at booting or
initialization stage. After that, they need allocate small order memory. The
best
is to just allocate order-0 memory.
If you have some other driver that fails because of large kmalloc failure,
you should fix it to use scatter-gather DMA or (if the hardware can't do
it) preallocate the memory when the driver is loaded.
I agree with you. Here the patch fixes the issue, where dm might allocate
big continuous memory after booting. Monkey testing triggers it by operating
menu Setting=Security=InstallfromSDcard.
hi, Yanmin Mikulas
Thanks for your nice comments. And sorry for confusing you as my
comment is not clear enough.
In android, there is a command dumpstate, it run many other commands to
collect information. In our scenario,
it run command vdc dump, and vdc uses socket to pass some parameters to
vold, then vold generates ioctl.
thanks.
We are catching all places in kernel where big memory allocation happens.
This patch is just to fix one case.
Thanks,
Yanmin
Mikulas
On Fri, 4 Jul 2014, xinhui.pan wrote:
KMALLOC_MAX_SIZE is too big, it easily cause memory fragmented and low
memory when param_kernel-data_size
is also big. That's not nice. So use vmalloc instead of kmalloc when size
is larger than (PAGE_SIZE 2).
There are other drivers using kmalloc to gain a big size memory. And that
cause our devices to hit hang and
many worse issues. We don't benefit much when using kmalloc in such
scenario.
Signed-off-by: xinhui.pan xinhuix@intel.com
Signed-off-by: yanmin.zhang yanmin_zh...@linux.intel.com
---
drivers/md/dm-ioctl.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 5152142..31c3af9 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1709,7 +1709,7 @@ static int copy_params(struct dm_ioctl __user *user,
struct dm_ioctl *param_kern
* Use kmalloc() rather than vmalloc() when we can.
*/
dmi = NULL;
- if (param_kernel-data_size = KMALLOC_MAX_SIZE) {
+ if (param_kernel-data_size = (PAGE_SIZE 2)) {
dmi = kmalloc(param_kernel-data_size, GFP_NOIO |
__GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN);
if (dmi)
*param_flags |= DM_PARAMS_KMALLOC;
--
1.7.9.5
--
dm-devel mailing list
dm-de...@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] md/dm-ioctl.c: optimize memory allocation in copy_params
KMALLOC_MAX_SIZE is too big, it easily cause memory fragmented and low memory
when param_kernel->data_size
is also big. That's not nice. So use vmalloc instead of kmalloc when size is
larger than (PAGE_SIZE << 2).
There are other drivers using kmalloc to gain a big size memory. And that cause
our devices to hit hang and
many worse issues. We don't benefit much when using kmalloc in such scenario.
Signed-off-by: xinhui.pan
Signed-off-by: yanmin.zhang
---
drivers/md/dm-ioctl.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 5152142..31c3af9 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1709,7 +1709,7 @@ static int copy_params(struct dm_ioctl __user *user,
struct dm_ioctl *param_kern
* Use kmalloc() rather than vmalloc() when we can.
*/
dmi = NULL;
- if (param_kernel->data_size <= KMALLOC_MAX_SIZE) {
+ if (param_kernel->data_size <= (PAGE_SIZE << 2)) {
dmi = kmalloc(param_kernel->data_size, GFP_NOIO | __GFP_NORETRY
| __GFP_NOMEMALLOC | __GFP_NOWARN);
if (dmi)
*param_flags |= DM_PARAMS_KMALLOC;
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] md/dm-ioctl.c: optimize memory allocation in copy_params
KMALLOC_MAX_SIZE is too big, it easily cause memory fragmented and low memory
when param_kernel-data_size
is also big. That's not nice. So use vmalloc instead of kmalloc when size is
larger than (PAGE_SIZE 2).
There are other drivers using kmalloc to gain a big size memory. And that cause
our devices to hit hang and
many worse issues. We don't benefit much when using kmalloc in such scenario.
Signed-off-by: xinhui.pan xinhuix@intel.com
Signed-off-by: yanmin.zhang yanmin_zh...@linux.intel.com
---
drivers/md/dm-ioctl.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 5152142..31c3af9 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1709,7 +1709,7 @@ static int copy_params(struct dm_ioctl __user *user,
struct dm_ioctl *param_kern
* Use kmalloc() rather than vmalloc() when we can.
*/
dmi = NULL;
- if (param_kernel-data_size = KMALLOC_MAX_SIZE) {
+ if (param_kernel-data_size = (PAGE_SIZE 2)) {
dmi = kmalloc(param_kernel-data_size, GFP_NOIO | __GFP_NORETRY
| __GFP_NOMEMALLOC | __GFP_NOWARN);
if (dmi)
*param_flags |= DM_PARAMS_KMALLOC;
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb/core/hub.c: return immediately when hub_port_init hits timedout
于 2014年03月07日 14:47, Greg KH 写道: > On Fri, Mar 07, 2014 at 02:15:48PM +0800, xinhui.pan wrote: >> From: "xinhui.pan" > > I doubt your name as a "." in it, right? > yes :) >> some devices go crasy, we can't resume it even after reset. > > I don't understand, what do you mean by this? What exactly does a > device do, and why does it do it? And what happens? > the device(modem) did not response. below is the call trace. [ 2950.647310] usb 1-1: reset high-speed USB device number 2 using ehci-intel-hsic [ 2957.535127] usb 1-1: DPM device timeout [ 2957.535153] 88003a1df930 0046 0001 88003a1dffd8 [ 2957.535170] 88003a14 88003a1dffd8 88003a1dffd8 830cb440 [ 2957.535187] 88003a14 833964c0 88003a1df960 00010003e2df [ 2957.535192] Call Trace: [ 2957.535221] [] schedule+0x29/0x70 [ 2957.535237] [] schedule_timeout+0x15b/0x300 [ 2957.535255] [] ? __internal_add_timer+0x130/0x130 [ 2957.535271] [] wait_for_completion_timeout+0xd7/0x120 [ 2957.535286] [] ? try_to_wake_up+0x2d0/0x2d0 [ 2957.535303] [] usb_start_wait_urb+0x7e/0x150 [ 2957.535318] [] usb_control_msg+0xc2/0x100 [ 2957.535334] [] hub_port_init+0x4d8/0xa90 [ 2957.535351] [] usb_reset_and_verify_device+0x102/0x430 [ 2957.535364] [] ? usb_get_status+0x8c/0xb0 [ 2957.535379] [] usb_port_resume+0x408/0x660 [ 2957.535393] [] ? schedule+0x29/0x70 [ 2957.535406] [] ? usb_dev_thaw+0x20/0x20 [ 2957.535419] [] generic_resume+0x1e/0x50 [ 2957.535431] [] ? get_parent_ip+0xd/0x50 [ 2957.535445] [] usb_resume_both+0x105/0x150 [ 2957.535458] [] usb_resume+0x1f/0xd0 [ 2957.535471] [] ? usb_dev_thaw+0x20/0x20 [ 2957.535484] [] usb_dev_resume+0x13/0x20 [ 2957.535499] [] dpm_run_callback+0x4e/0x80 [ 2957.535513] [] device_resume+0xf3/0x260 [ 2957.535526] [] ? dpm_wd_set+0x60/0x60 [ 2957.535541] [] async_resume+0x1d/0x50 [ 2957.53] [] async_run_entry_fn+0x39/0x120 [ 2957.535572] [] process_one_work+0x177/0x490 [ 2957.535585] [] worker_thread+0x123/0x380 [ 2957.535599] [] ? rescuer_thread+0x310/0x310 [ 2957.535613] [] kthread+0xd3/0xe0 [ 2957.535625] [] ? get_parent_ip+0xd/0x50 [ 2957.535642] [] ? kthread_create_on_node+0x110/0x110 [ 2957.535657] [] ret_from_fork+0x7c/0xb0 [ 2957.535672] [] ? kthread_create_on_node+0x110/0x110 the device can't resume during the kernel resume progress. perhaps it has some issue to deal with and has to reset itself. waiting for it to be completed seems wasting time. >> This case will cause timeout again and again. What is worse, if there >> is a watchdog, panic will be generated as timer expires. > > A watchdog where? > [ 2957.535526] [] ? dpm_wd_set+0x60/0x60 I also doubt the use of watchdog. but we really know some not good devices make the kernel in risk. >> To prevent this, we just return -ENODEV immediately. Later it will be >> re-enumerated. > > What will cause it to be re-enumerated? > > confused, > after return, hub_port_logical_disconnect will be called. to my knowledge it will re-enumerate if there actually is a device attached. > greg k-h > thanks :) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] usb/core/hub.c: return immediately when hub_port_init hits timedout
From: "xinhui.pan"
some devices go crasy, we can't resume it even after reset. This case will
cause timeout again and again. What is worse, if there is a watchdog,
panic will be generated as timer expires. To prevent this, we just return
-ENODEV immediately. Later it will be re-enumerated.
Signed-off-by: xinhui.pan
---
drivers/usb/core/hub.c |6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 64ea219..c5d0d8d 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4141,7 +4141,7 @@ hub_port_init (struct usb_hub *hub, struct usb_device
*udev, int port1,
r = -EPROTO;
break;
}
- if (r == 0)
+ if (r == 0 || r == -ETIMEDOUT)
break;
}
udev->descriptor.bMaxPacketSize0 =
@@ -4161,6 +4161,10 @@ hub_port_init (struct usb_hub *hub, struct usb_device
*udev, int port1,
if (r != -ENODEV)
dev_err(>dev, "device descriptor
read/64, error %d\n",
r);
+ if (r == -ETIMEDOUT) {
+ retval = -ENODEV;
+ goto fail;
+ }
retval = -EMSGSIZE;
continue;
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] usb/core/hub.c: return immediately when hub_port_init hits timedout
From: xinhui.pan xinhuix@intel.com
some devices go crasy, we can't resume it even after reset. This case will
cause timeout again and again. What is worse, if there is a watchdog,
panic will be generated as timer expires. To prevent this, we just return
-ENODEV immediately. Later it will be re-enumerated.
Signed-off-by: xinhui.pan xinhuix@intel.com
---
drivers/usb/core/hub.c |6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 64ea219..c5d0d8d 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4141,7 +4141,7 @@ hub_port_init (struct usb_hub *hub, struct usb_device
*udev, int port1,
r = -EPROTO;
break;
}
- if (r == 0)
+ if (r == 0 || r == -ETIMEDOUT)
break;
}
udev-descriptor.bMaxPacketSize0 =
@@ -4161,6 +4161,10 @@ hub_port_init (struct usb_hub *hub, struct usb_device
*udev, int port1,
if (r != -ENODEV)
dev_err(udev-dev, device descriptor
read/64, error %d\n,
r);
+ if (r == -ETIMEDOUT) {
+ retval = -ENODEV;
+ goto fail;
+ }
retval = -EMSGSIZE;
continue;
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb/core/hub.c: return immediately when hub_port_init hits timedout
于 2014年03月07日 14:47, Greg KH 写道: On Fri, Mar 07, 2014 at 02:15:48PM +0800, xinhui.pan wrote: From: xinhui.pan xinhuix@intel.com I doubt your name as a . in it, right? yes :) some devices go crasy, we can't resume it even after reset. I don't understand, what do you mean by this? What exactly does a device do, and why does it do it? And what happens? the device(modem) did not response. below is the call trace. [ 2950.647310] usb 1-1: reset high-speed USB device number 2 using ehci-intel-hsic [ 2957.535127] usb 1-1: DPM device timeout [ 2957.535153] 88003a1df930 0046 0001 88003a1dffd8 [ 2957.535170] 88003a14 88003a1dffd8 88003a1dffd8 830cb440 [ 2957.535187] 88003a14 833964c0 88003a1df960 00010003e2df [ 2957.535192] Call Trace: [ 2957.535221] [82843289] schedule+0x29/0x70 [ 2957.535237] [828404bb] schedule_timeout+0x15b/0x300 [ 2957.535255] [8207d190] ? __internal_add_timer+0x130/0x130 [ 2957.535271] [828423d7] wait_for_completion_timeout+0xd7/0x120 [ 2957.535286] [820a3130] ? try_to_wake_up+0x2d0/0x2d0 [ 2957.535303] [824f756e] usb_start_wait_urb+0x7e/0x150 [ 2957.535318] [824f7872] usb_control_msg+0xc2/0x100 [ 2957.535334] [824ed268] hub_port_init+0x4d8/0xa90 [ 2957.535351] [824ed982] usb_reset_and_verify_device+0x102/0x430 [ 2957.535364] [824f79bc] ? usb_get_status+0x8c/0xb0 [ 2957.535379] [824f00d8] usb_port_resume+0x408/0x660 [ 2957.535393] [82843289] ? schedule+0x29/0x70 [ 2957.535406] [824ea1a0] ? usb_dev_thaw+0x20/0x20 [ 2957.535419] [8250356e] generic_resume+0x1e/0x50 [ 2957.535431] [820a11ad] ? get_parent_ip+0xd/0x50 [ 2957.535445] [824f9ed5] usb_resume_both+0x105/0x150 [ 2957.535458] [824facff] usb_resume+0x1f/0xd0 [ 2957.535471] [824ea1a0] ? usb_dev_thaw+0x20/0x20 [ 2957.535484] [824ea1b3] usb_dev_resume+0x13/0x20 [ 2957.535499] [823fa86e] dpm_run_callback+0x4e/0x80 [ 2957.535513] [823fb373] device_resume+0xf3/0x260 [ 2957.535526] [823fa770] ? dpm_wd_set+0x60/0x60 [ 2957.535541] [823fb4fd] async_resume+0x1d/0x50 [ 2957.53] [8209a439] async_run_entry_fn+0x39/0x120 [ 2957.535572] [8208c767] process_one_work+0x177/0x490 [ 2957.535585] [8208cef3] worker_thread+0x123/0x380 [ 2957.535599] [8208cdd0] ? rescuer_thread+0x310/0x310 [ 2957.535613] [82093a63] kthread+0xd3/0xe0 [ 2957.535625] [820a11ad] ? get_parent_ip+0xd/0x50 [ 2957.535642] [82093990] ? kthread_create_on_node+0x110/0x110 [ 2957.535657] [8284a59c] ret_from_fork+0x7c/0xb0 [ 2957.535672] [82093990] ? kthread_create_on_node+0x110/0x110 the device can't resume during the kernel resume progress. perhaps it has some issue to deal with and has to reset itself. waiting for it to be completed seems wasting time. This case will cause timeout again and again. What is worse, if there is a watchdog, panic will be generated as timer expires. A watchdog where? [ 2957.535526] [823fa770] ? dpm_wd_set+0x60/0x60 I also doubt the use of watchdog. but we really know some not good devices make the kernel in risk. To prevent this, we just return -ENODEV immediately. Later it will be re-enumerated. What will cause it to be re-enumerated? confused, after return, hub_port_logical_disconnect will be called. to my knowledge it will re-enumerate if there actually is a device attached. greg k-h thanks :) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio-intel-mid: fix the incorrect return of idle callback
On Wed, 29 Jan 2014 13:06, David Cohen wrote:
> On Wed, Jan 29, 2014 at 01:52:30PM -0600, Felipe Balbi wrote:
>> On Wed, Jan 29, 2014 at 11:12:32AM -0800, David Cohen wrote:
>>> On Wed, Jan 29, 2014 at 03:23:40PM +0800, xinhui.pan wrote:
>>>>
>>>> 于 2014年01月29日 08:13, David Cohen 写道:
>>>>> On Tue, Jan 28, 2014 at 12:12:06PM -0600, Felipe Balbi wrote:
>>>>>> On Tue, Jan 28, 2014 at 09:24:13AM -0800, David Cohen wrote:
>>>>>>> On Tue, Jan 28, 2014 at 10:49:37AM -0600, Felipe Balbi wrote:
>>>>>>>> On Tue, Jan 28, 2014 at 04:50:57PM +0800, xinhui.pan wrote:
>>>>>>>>> From: "xinhui.pan"
>>>>>>>>>
>>>>>>>>> intel_gpio_runtime_idle should return correct error code if it do
>>>>>>>>> fail.
>>>>>>>>> make it more correct even though -EBUSY is the most possible return
>>>>>>>>> value.
>>>>>>>>>
>>>>>>>>> Signed-off-by: bo.he
>>>>>>>>> Signed-off-by: xinhui.pan
>>>>>>>>> ---
>>>>>>>>> drivers/gpio/gpio-intel-mid.c |4 +++-
>>>>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpio/gpio-intel-mid.c
>>>>>>>>> b/drivers/gpio/gpio-intel-mid.c
>>>>>>>>> index d1b50ef..05749a3 100644
>>>>>>>>> --- a/drivers/gpio/gpio-intel-mid.c
>>>>>>>>> +++ b/drivers/gpio/gpio-intel-mid.c
>>>>>>>>> @@ -394,7 +394,9 @@ static const struct irq_domain_ops
>>>>>>>>> intel_gpio_irq_ops = {
>>>>>>>>>
>>>>>>>>> static int intel_gpio_runtime_idle(struct device *dev)
>>>>>>>>> {
>>>>>>>>> - pm_schedule_suspend(dev, 500);
>>>>>>>>> + int err = pm_schedule_suspend(dev, 500);
>>>>>>>>> + if (err)
>>>>>>>>> + return err;
>>>>>>>>> return -EBUSY;
>>>>>>>>
>>>>>>>> wait, is it only me or this would look a lot better as:
>>>>>>>>
>>>>>>>> static int intel_gpio_runtime_idle(struct device *dev)
>>>>>>>> {
>>>>>>>>return pm_schedule_suspend(dev, 500);
>>>>>>>> }
>>>>>>>
>>>>>>> The reply to your suggestion is probably in this commit :)
>>>>>>>
>>>>>>> ---
>>>>>>> commit 45f0a85c8258741d11bda25c0a5669c06267204a
>>>>>>> Author: Rafael J. Wysocki
>>>>>>> Date: Mon Jun 3 21:49:52 2013 +0200
>>>>>>>
>>>>>>> PM / Runtime: Rework the "runtime idle" helper routine
>>>>>>> ---
>>>>>>>
>>>>>>> We won't return 0 from here.
>>>>>>
>>>>>> so you never want to return 0, why don't you, then:
>>>>>>
>>>>>> static int intel_gpio_runtime_idle(struct device *dev)
>>>>>> {
>>>>>> pm_schedule_suspend(dev, 500);
>>>>>> return -EBUSY;
>>>>>> }
>>>>>
>>>>> That's how it is currently :)
>>>>>
>>>>> But this patch is making the function to return a different code in case
>>>>> of error. IMHO there is not much fuctional gain with it, but I see
>>>>> perhaps one extra info for tracing during development.
>>>>>
>>>>> Anyway, I'll let Xinhui to do further comment since he's the author.
>>>>>
>>>>> Br, David
>>>>>
>>>> hi ,David & Balbi
>>>> I checked several drivers yesterday to see how they use
>>>> pm_schedule_suspend
>>>> then found one bug in i2c. Also I noticed gpio.
>>>> I think returning a correct error code is important.So I change -EBUSY
>>>> to *err*. To be honest,current code works well.
>>>
>>> In my experience, when I'm using fancy things like lauterbach a proper
>>> error code may save couple of minutes in my life :)
>>>
>>> I keep my ack here.
>>
>> fair enough, sorry for the noise ;-) It could still be simplified a bit:
>>
>> return err ?: -EBUSY;
>
> Agreed :)
> Xinhui, could we have this suggestion in your patch?
>
> Br, David
>
Hi all,
I am xinhui pan. Thanks to the VPN problem, I can't access Intel's network.
So I have to send you this email by personal email address.
I am on Spring Festival vacation until Feb 9th. Sorry for that.
Your suggestion is very nice,thanks :)
I will generate V2 patch ASAP when my vocation is over. Thanks for all your
help.
>>
>> --
>> balbi
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio-intel-mid: fix the incorrect return of idle callback
On Wed, 29 Jan 2014 13:06, David Cohen wrote:
On Wed, Jan 29, 2014 at 01:52:30PM -0600, Felipe Balbi wrote:
On Wed, Jan 29, 2014 at 11:12:32AM -0800, David Cohen wrote:
On Wed, Jan 29, 2014 at 03:23:40PM +0800, xinhui.pan wrote:
于 2014年01月29日 08:13, David Cohen 写道:
On Tue, Jan 28, 2014 at 12:12:06PM -0600, Felipe Balbi wrote:
On Tue, Jan 28, 2014 at 09:24:13AM -0800, David Cohen wrote:
On Tue, Jan 28, 2014 at 10:49:37AM -0600, Felipe Balbi wrote:
On Tue, Jan 28, 2014 at 04:50:57PM +0800, xinhui.pan wrote:
From: xinhui.pan xinhuix@intel.com
intel_gpio_runtime_idle should return correct error code if it do
fail.
make it more correct even though -EBUSY is the most possible return
value.
Signed-off-by: bo.he bo...@intel.com
Signed-off-by: xinhui.pan xinhuix@intel.com
---
drivers/gpio/gpio-intel-mid.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-intel-mid.c
b/drivers/gpio/gpio-intel-mid.c
index d1b50ef..05749a3 100644
--- a/drivers/gpio/gpio-intel-mid.c
+++ b/drivers/gpio/gpio-intel-mid.c
@@ -394,7 +394,9 @@ static const struct irq_domain_ops
intel_gpio_irq_ops = {
static int intel_gpio_runtime_idle(struct device *dev)
{
- pm_schedule_suspend(dev, 500);
+ int err = pm_schedule_suspend(dev, 500);
+ if (err)
+ return err;
return -EBUSY;
wait, is it only me or this would look a lot better as:
static int intel_gpio_runtime_idle(struct device *dev)
{
return pm_schedule_suspend(dev, 500);
}
The reply to your suggestion is probably in this commit :)
---
commit 45f0a85c8258741d11bda25c0a5669c06267204a
Author: Rafael J. Wysocki rafael.j.wyso...@intel.com
Date: Mon Jun 3 21:49:52 2013 +0200
PM / Runtime: Rework the runtime idle helper routine
---
We won't return 0 from here.
so you never want to return 0, why don't you, then:
static int intel_gpio_runtime_idle(struct device *dev)
{
pm_schedule_suspend(dev, 500);
return -EBUSY;
}
That's how it is currently :)
But this patch is making the function to return a different code in case
of error. IMHO there is not much fuctional gain with it, but I see
perhaps one extra info for tracing during development.
Anyway, I'll let Xinhui to do further comment since he's the author.
Br, David
hi ,David Balbi
I checked several drivers yesterday to see how they use
pm_schedule_suspend
then found one bug in i2c. Also I noticed gpio.
I think returning a correct error code is important.So I change -EBUSY
to *err*. To be honest,current code works well.
In my experience, when I'm using fancy things like lauterbach a proper
error code may save couple of minutes in my life :)
I keep my ack here.
fair enough, sorry for the noise ;-) It could still be simplified a bit:
return err ?: -EBUSY;
Agreed :)
Xinhui, could we have this suggestion in your patch?
Br, David
Hi all,
I am xinhui pan. Thanks to the VPN problem, I can't access Intel's network.
So I have to send you this email by personal email address.
I am on Spring Festival vacation until Feb 9th. Sorry for that.
Your suggestion is very nice,thanks :)
I will generate V2 patch ASAP when my vocation is over. Thanks for all your
help.
--
balbi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i2c-designware-pcidrv: fix the incorrect return of idle callback
于 2014年01月29日 16:35, Mika Westerberg 写道: > On Tue, Jan 28, 2014 at 07:30:35PM +0100, Wolfram Sang wrote: >> On Tue, Jan 28, 2014 at 01:48:28PM +0800, xinhui.pan wrote: >>> From: "xinhui.pan" >>> >>> i2c_dw_pci_runtime_idle should return -EBUSY rather than zero if it do >>> success. >> >> I don't understand... >> >>> Otherwise rpm_idle will call pm_suspend again and that may cause >>> pm_schedule_suspend delay invalidate. >>> >>> Signed-off-by: bo.he >>> Signed-off-by: xinhui.pan >>> --- >>> drivers/i2c/busses/i2c-designware-pcidrv.c |4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c >>> b/drivers/i2c/busses/i2c-designware-pcidrv.c >>> index f6ed06c..96e81f6 100644 >>> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c >>> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c >>> @@ -190,8 +190,8 @@ static int i2c_dw_pci_runtime_idle(struct device *dev) >>> int err = pm_schedule_suspend(dev, 500); >>> dev_dbg(dev, "runtime_idle called\n"); >>> >>> - if (err != 0) >>> - return 0; >>> + if (err) >>> + return err; >>> return -EBUSY; >> >> ... it does return EBUSY when pm_schedule_suspend() succeeds? It only >> returns 0 if it does not succeed (for which I don't know if this is an >> apropriate behaviour). Mika? > > If I understand correctly, pm_schedule_suspend(dev, 500) is there because > we want to runtime suspend in 500 ms. It then returns -EBUSY to prevent PM > runtime from carrying on suspend on it's own. However, I have no idea where > this magical 500 ms requirement comes from. > > If we fail to schedule suspend we let the PM core to do whatever it thinks > suitable (in this case I suppose it suspends the device). > Hi ,Mika If the callback returns 0,it means pm_schedule_suspend fails, also means rpm_check_suspend_allowed(pm_schedule_suspend calls it) returns nonzero value.As a result,rpm_suspend will be called by rpm_idle. However in rpm_idle, rpm_check_suspend_allowed is called at first,too. and the return value is treated as it is.But rpm_idle just returns without doing anything(rpm_suspend is not called). in both case above,why goes in different ways? I am confused. > I think the whole idle dance could be replaced with a use of runtime PM > autosuspend, just like we do in the platform version of the driver. > > Xinghui, > > Is this a real problem that you are trying to solve? > To be honest,we got many panic when testing. But is not caused by this driver I think. while checking problems,we found these confusing codes by accident. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i2c-designware-pcidrv: fix the incorrect return of idle callback
于 2014年01月29日 16:35, Mika Westerberg 写道: On Tue, Jan 28, 2014 at 07:30:35PM +0100, Wolfram Sang wrote: On Tue, Jan 28, 2014 at 01:48:28PM +0800, xinhui.pan wrote: From: xinhui.pan xinhuix@intel.com i2c_dw_pci_runtime_idle should return -EBUSY rather than zero if it do success. I don't understand... Otherwise rpm_idle will call pm_suspend again and that may cause pm_schedule_suspend delay invalidate. Signed-off-by: bo.he bo...@intel.com Signed-off-by: xinhui.pan xinhuix@intel.com --- drivers/i2c/busses/i2c-designware-pcidrv.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c index f6ed06c..96e81f6 100644 --- a/drivers/i2c/busses/i2c-designware-pcidrv.c +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c @@ -190,8 +190,8 @@ static int i2c_dw_pci_runtime_idle(struct device *dev) int err = pm_schedule_suspend(dev, 500); dev_dbg(dev, runtime_idle called\n); - if (err != 0) - return 0; + if (err) + return err; return -EBUSY; ... it does return EBUSY when pm_schedule_suspend() succeeds? It only returns 0 if it does not succeed (for which I don't know if this is an apropriate behaviour). Mika? If I understand correctly, pm_schedule_suspend(dev, 500) is there because we want to runtime suspend in 500 ms. It then returns -EBUSY to prevent PM runtime from carrying on suspend on it's own. However, I have no idea where this magical 500 ms requirement comes from. If we fail to schedule suspend we let the PM core to do whatever it thinks suitable (in this case I suppose it suspends the device). Hi ,Mika If the callback returns 0,it means pm_schedule_suspend fails, also means rpm_check_suspend_allowed(pm_schedule_suspend calls it) returns nonzero value.As a result,rpm_suspend will be called by rpm_idle. However in rpm_idle, rpm_check_suspend_allowed is called at first,too. and the return value is treated as it is.But rpm_idle just returns without doing anything(rpm_suspend is not called). in both case above,why goes in different ways? I am confused. I think the whole idle dance could be replaced with a use of runtime PM autosuspend, just like we do in the platform version of the driver. Xinghui, Is this a real problem that you are trying to solve? To be honest,we got many panic when testing. But is not caused by this driver I think. while checking problems,we found these confusing codes by accident. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i2c-designware-pcidrv: fix the incorrect return of idle callback
于 2014年01月29日 10:03, xinhui.pan 写道: > >> On Tue, Jan 28, 2014 at 01:48:28PM +0800, xinhui.pan wrote: >>> From: "xinhui.pan" >>> >>> i2c_dw_pci_runtime_idle should return -EBUSY rather than zero if it do >>> success. >> >> I don't understand... >> > Sorry for my poor English. > Even if i2c_dw_pci_runtime_idle succeed ,it should return -EBUSY. >>> Otherwise rpm_idle will call pm_suspend again and that may cause >>> pm_schedule_suspend delay invalidate. >>> >>> Signed-off-by: bo.he >>> Signed-off-by: xinhui.pan >>> --- >>> drivers/i2c/busses/i2c-designware-pcidrv.c |4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c >>> b/drivers/i2c/busses/i2c-designware-pcidrv.c >>> index f6ed06c..96e81f6 100644 >>> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c >>> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c >>> @@ -190,8 +190,8 @@ static int i2c_dw_pci_runtime_idle(struct device *dev) >>> int err = pm_schedule_suspend(dev, 500); >>> dev_dbg(dev, "runtime_idle called\n"); >>> >>> - if (err != 0) >>> - return 0; >>> + if (err) >>> + return err; >>> return -EBUSY; >> >> ... it does return EBUSY when pm_schedule_suspend() succeeds? It only >> returns 0 if it does not succeed (for which I don't know if this is an >> apropriate behaviour). Mika? >> hi , I found one sentence in /Documentation/power/runtime_pm.txt "If there is no idle callback, or if the callback returns 0, then the PM core will attempt to carry out a runtime suspend of the device, also respecting devices configured for autosuspend." so is this a right way to prevent this? Br. xinhui >>> } >>> >>> -- >>> 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio-intel-mid: fix the incorrect return of idle callback
于 2014年01月29日 08:13, David Cohen 写道:
> On Tue, Jan 28, 2014 at 12:12:06PM -0600, Felipe Balbi wrote:
>> On Tue, Jan 28, 2014 at 09:24:13AM -0800, David Cohen wrote:
>>> On Tue, Jan 28, 2014 at 10:49:37AM -0600, Felipe Balbi wrote:
>>>> On Tue, Jan 28, 2014 at 04:50:57PM +0800, xinhui.pan wrote:
>>>>> From: "xinhui.pan"
>>>>>
>>>>> intel_gpio_runtime_idle should return correct error code if it do fail.
>>>>> make it more correct even though -EBUSY is the most possible return value.
>>>>>
>>>>> Signed-off-by: bo.he
>>>>> Signed-off-by: xinhui.pan
>>>>> ---
>>>>> drivers/gpio/gpio-intel-mid.c |4 +++-
>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpio/gpio-intel-mid.c b/drivers/gpio/gpio-intel-mid.c
>>>>> index d1b50ef..05749a3 100644
>>>>> --- a/drivers/gpio/gpio-intel-mid.c
>>>>> +++ b/drivers/gpio/gpio-intel-mid.c
>>>>> @@ -394,7 +394,9 @@ static const struct irq_domain_ops intel_gpio_irq_ops
>>>>> = {
>>>>>
>>>>> static int intel_gpio_runtime_idle(struct device *dev)
>>>>> {
>>>>> - pm_schedule_suspend(dev, 500);
>>>>> + int err = pm_schedule_suspend(dev, 500);
>>>>> + if (err)
>>>>> + return err;
>>>>> return -EBUSY;
>>>>
>>>> wait, is it only me or this would look a lot better as:
>>>>
>>>> static int intel_gpio_runtime_idle(struct device *dev)
>>>> {
>>>>return pm_schedule_suspend(dev, 500);
>>>> }
>>>
>>> The reply to your suggestion is probably in this commit :)
>>>
>>> ---
>>> commit 45f0a85c8258741d11bda25c0a5669c06267204a
>>> Author: Rafael J. Wysocki
>>> Date: Mon Jun 3 21:49:52 2013 +0200
>>>
>>> PM / Runtime: Rework the "runtime idle" helper routine
>>> ---
>>>
>>> We won't return 0 from here.
>>
>> so you never want to return 0, why don't you, then:
>>
>> static int intel_gpio_runtime_idle(struct device *dev)
>> {
>> pm_schedule_suspend(dev, 500);
>> return -EBUSY;
>> }
>
> That's how it is currently :)
>
> But this patch is making the function to return a different code in case
> of error. IMHO there is not much fuctional gain with it, but I see
> perhaps one extra info for tracing during development.
>
> Anyway, I'll let Xinhui to do further comment since he's the author.
>
> Br, David
>
hi ,David & Balbi
I checked several drivers yesterday to see how they use pm_schedule_suspend
then found one bug in i2c. Also I noticed gpio.
I think returning a correct error code is important.So I change -EBUSY
to *err*. To be honest,current code works well.
>>
>> just like drivers/tty/serial/mfd.c::serial_hsu_runtime_idle() is doing ?
>>
>> --
>> balbi
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i2c-designware-pcidrv: fix the incorrect return of idle callback
> On Tue, Jan 28, 2014 at 01:48:28PM +0800, xinhui.pan wrote: >> From: "xinhui.pan" >> >> i2c_dw_pci_runtime_idle should return -EBUSY rather than zero if it do >> success. > > I don't understand... > Sorry for my poor English. Even if i2c_dw_pci_runtime_idle succeed ,it should return -EBUSY. >> Otherwise rpm_idle will call pm_suspend again and that may cause >> pm_schedule_suspend delay invalidate. >> >> Signed-off-by: bo.he >> Signed-off-by: xinhui.pan >> --- >> drivers/i2c/busses/i2c-designware-pcidrv.c |4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c >> b/drivers/i2c/busses/i2c-designware-pcidrv.c >> index f6ed06c..96e81f6 100644 >> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c >> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c >> @@ -190,8 +190,8 @@ static int i2c_dw_pci_runtime_idle(struct device *dev) >> int err = pm_schedule_suspend(dev, 500); >> dev_dbg(dev, "runtime_idle called\n"); >> >> -if (err != 0) >> -return 0; >> +if (err) >> +return err; >> return -EBUSY; > > ... it does return EBUSY when pm_schedule_suspend() succeeds? It only > returns 0 if it does not succeed (for which I don't know if this is an > apropriate behaviour). Mika? > >> } >> >> -- >> 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] gpio-intel-mid: fix the incorrect return of idle callback
From: "xinhui.pan"
intel_gpio_runtime_idle should return correct error code if it do fail.
make it more correct even though -EBUSY is the most possible return value.
Signed-off-by: bo.he
Signed-off-by: xinhui.pan
---
drivers/gpio/gpio-intel-mid.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-intel-mid.c b/drivers/gpio/gpio-intel-mid.c
index d1b50ef..05749a3 100644
--- a/drivers/gpio/gpio-intel-mid.c
+++ b/drivers/gpio/gpio-intel-mid.c
@@ -394,7 +394,9 @@ static const struct irq_domain_ops intel_gpio_irq_ops = {
static int intel_gpio_runtime_idle(struct device *dev)
{
- pm_schedule_suspend(dev, 500);
+ int err = pm_schedule_suspend(dev, 500);
+ if (err)
+ return err;
return -EBUSY;
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] gpio-intel-mid: fix the incorrect return of idle callback
From: xinhui.pan xinhuix@intel.com
intel_gpio_runtime_idle should return correct error code if it do fail.
make it more correct even though -EBUSY is the most possible return value.
Signed-off-by: bo.he bo...@intel.com
Signed-off-by: xinhui.pan xinhuix@intel.com
---
drivers/gpio/gpio-intel-mid.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-intel-mid.c b/drivers/gpio/gpio-intel-mid.c
index d1b50ef..05749a3 100644
--- a/drivers/gpio/gpio-intel-mid.c
+++ b/drivers/gpio/gpio-intel-mid.c
@@ -394,7 +394,9 @@ static const struct irq_domain_ops intel_gpio_irq_ops = {
static int intel_gpio_runtime_idle(struct device *dev)
{
- pm_schedule_suspend(dev, 500);
+ int err = pm_schedule_suspend(dev, 500);
+ if (err)
+ return err;
return -EBUSY;
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i2c-designware-pcidrv: fix the incorrect return of idle callback
On Tue, Jan 28, 2014 at 01:48:28PM +0800, xinhui.pan wrote: From: xinhui.pan xinhuix@intel.com i2c_dw_pci_runtime_idle should return -EBUSY rather than zero if it do success. I don't understand... Sorry for my poor English. Even if i2c_dw_pci_runtime_idle succeed ,it should return -EBUSY. Otherwise rpm_idle will call pm_suspend again and that may cause pm_schedule_suspend delay invalidate. Signed-off-by: bo.he bo...@intel.com Signed-off-by: xinhui.pan xinhuix@intel.com --- drivers/i2c/busses/i2c-designware-pcidrv.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c index f6ed06c..96e81f6 100644 --- a/drivers/i2c/busses/i2c-designware-pcidrv.c +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c @@ -190,8 +190,8 @@ static int i2c_dw_pci_runtime_idle(struct device *dev) int err = pm_schedule_suspend(dev, 500); dev_dbg(dev, runtime_idle called\n); -if (err != 0) -return 0; +if (err) +return err; return -EBUSY; ... it does return EBUSY when pm_schedule_suspend() succeeds? It only returns 0 if it does not succeed (for which I don't know if this is an apropriate behaviour). Mika? } -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio-intel-mid: fix the incorrect return of idle callback
于 2014年01月29日 08:13, David Cohen 写道:
On Tue, Jan 28, 2014 at 12:12:06PM -0600, Felipe Balbi wrote:
On Tue, Jan 28, 2014 at 09:24:13AM -0800, David Cohen wrote:
On Tue, Jan 28, 2014 at 10:49:37AM -0600, Felipe Balbi wrote:
On Tue, Jan 28, 2014 at 04:50:57PM +0800, xinhui.pan wrote:
From: xinhui.pan xinhuix@intel.com
intel_gpio_runtime_idle should return correct error code if it do fail.
make it more correct even though -EBUSY is the most possible return value.
Signed-off-by: bo.he bo...@intel.com
Signed-off-by: xinhui.pan xinhuix@intel.com
---
drivers/gpio/gpio-intel-mid.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-intel-mid.c b/drivers/gpio/gpio-intel-mid.c
index d1b50ef..05749a3 100644
--- a/drivers/gpio/gpio-intel-mid.c
+++ b/drivers/gpio/gpio-intel-mid.c
@@ -394,7 +394,9 @@ static const struct irq_domain_ops intel_gpio_irq_ops
= {
static int intel_gpio_runtime_idle(struct device *dev)
{
- pm_schedule_suspend(dev, 500);
+ int err = pm_schedule_suspend(dev, 500);
+ if (err)
+ return err;
return -EBUSY;
wait, is it only me or this would look a lot better as:
static int intel_gpio_runtime_idle(struct device *dev)
{
return pm_schedule_suspend(dev, 500);
}
The reply to your suggestion is probably in this commit :)
---
commit 45f0a85c8258741d11bda25c0a5669c06267204a
Author: Rafael J. Wysocki rafael.j.wyso...@intel.com
Date: Mon Jun 3 21:49:52 2013 +0200
PM / Runtime: Rework the runtime idle helper routine
---
We won't return 0 from here.
so you never want to return 0, why don't you, then:
static int intel_gpio_runtime_idle(struct device *dev)
{
pm_schedule_suspend(dev, 500);
return -EBUSY;
}
That's how it is currently :)
But this patch is making the function to return a different code in case
of error. IMHO there is not much fuctional gain with it, but I see
perhaps one extra info for tracing during development.
Anyway, I'll let Xinhui to do further comment since he's the author.
Br, David
hi ,David Balbi
I checked several drivers yesterday to see how they use pm_schedule_suspend
then found one bug in i2c. Also I noticed gpio.
I think returning a correct error code is important.So I change -EBUSY
to *err*. To be honest,current code works well.
just like drivers/tty/serial/mfd.c::serial_hsu_runtime_idle() is doing ?
--
balbi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i2c-designware-pcidrv: fix the incorrect return of idle callback
于 2014年01月29日 10:03, xinhui.pan 写道: On Tue, Jan 28, 2014 at 01:48:28PM +0800, xinhui.pan wrote: From: xinhui.pan xinhuix@intel.com i2c_dw_pci_runtime_idle should return -EBUSY rather than zero if it do success. I don't understand... Sorry for my poor English. Even if i2c_dw_pci_runtime_idle succeed ,it should return -EBUSY. Otherwise rpm_idle will call pm_suspend again and that may cause pm_schedule_suspend delay invalidate. Signed-off-by: bo.he bo...@intel.com Signed-off-by: xinhui.pan xinhuix@intel.com --- drivers/i2c/busses/i2c-designware-pcidrv.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c index f6ed06c..96e81f6 100644 --- a/drivers/i2c/busses/i2c-designware-pcidrv.c +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c @@ -190,8 +190,8 @@ static int i2c_dw_pci_runtime_idle(struct device *dev) int err = pm_schedule_suspend(dev, 500); dev_dbg(dev, runtime_idle called\n); - if (err != 0) - return 0; + if (err) + return err; return -EBUSY; ... it does return EBUSY when pm_schedule_suspend() succeeds? It only returns 0 if it does not succeed (for which I don't know if this is an apropriate behaviour). Mika? hi , I found one sentence in /Documentation/power/runtime_pm.txt If there is no idle callback, or if the callback returns 0, then the PM core will attempt to carry out a runtime suspend of the device, also respecting devices configured for autosuspend. so is this a right way to prevent this? Br. xinhui } -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] i2c-designware-pcidrv: fix the incorrect return of idle callback
From: "xinhui.pan" i2c_dw_pci_runtime_idle should return -EBUSY rather than zero if it do success. Otherwise rpm_idle will call pm_suspend again and that may cause pm_schedule_suspend delay invalidate. Signed-off-by: bo.he Signed-off-by: xinhui.pan --- drivers/i2c/busses/i2c-designware-pcidrv.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c index f6ed06c..96e81f6 100644 --- a/drivers/i2c/busses/i2c-designware-pcidrv.c +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c @@ -190,8 +190,8 @@ static int i2c_dw_pci_runtime_idle(struct device *dev) int err = pm_schedule_suspend(dev, 500); dev_dbg(dev, "runtime_idle called\n"); - if (err != 0) - return 0; + if (err) + return err; return -EBUSY; } -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] i2c-designware-pcidrv: fix the incorrect return of idle callback
From: xinhui.pan xinhuix@intel.com i2c_dw_pci_runtime_idle should return -EBUSY rather than zero if it do success. Otherwise rpm_idle will call pm_suspend again and that may cause pm_schedule_suspend delay invalidate. Signed-off-by: bo.he bo...@intel.com Signed-off-by: xinhui.pan xinhuix@intel.com --- drivers/i2c/busses/i2c-designware-pcidrv.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c index f6ed06c..96e81f6 100644 --- a/drivers/i2c/busses/i2c-designware-pcidrv.c +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c @@ -190,8 +190,8 @@ static int i2c_dw_pci_runtime_idle(struct device *dev) int err = pm_schedule_suspend(dev, 500); dev_dbg(dev, runtime_idle called\n); - if (err != 0) - return 0; + if (err) + return err; return -EBUSY; } -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
