Re: [PATCH v2] input: don't call input_dev_release_keys() in resume
Hi Dmitry,
On 03:14 Tue 26 Nov , Dmitry Torokhov wrote:
> Hi Oskar,
>
> On Fri, Nov 22, 2013 at 02:27:04PM +0100, Oskar Andero wrote:
> > From: Aleksej Makarov
> >
> > When waking up the platform by pressing a specific key, sending a
> > release on that key makes it impossible to react on the event in
> > user-space. This is fixed by moving the input_reset_device() call to
> > resume instead.
> >
> > Cc: Dmitry Torokhov
> > Reviewed-by: Radovan Lekanovic
> > Signed-off-by: Aleksej Makarov
> > Signed-off-by: Oskar Andero
> > ---
> > drivers/input/input.c | 11 +--
> > 1 file changed, 1 insertion(+), 10 deletions(-)
> >
> > diff --git a/drivers/input/input.c b/drivers/input/input.c
> > index 846ccdd..511d490 100644
> > --- a/drivers/input/input.c
> > +++ b/drivers/input/input.c
> > @@ -1676,22 +1676,13 @@ static int input_dev_suspend(struct device *dev)
> > {
> > struct input_dev *input_dev = to_input_dev(dev);
> >
> > - mutex_lock(_dev->mutex);
> > -
> > - if (input_dev->users)
> > - input_dev_toggle(input_dev, false);
> > -
> > - mutex_unlock(_dev->mutex);
> > + input_reset_device(input_dev);
> >
> > return 0;
> > }
> >
> > static int input_dev_resume(struct device *dev)
> > {
> > - struct input_dev *input_dev = to_input_dev(dev);
> > -
> > - input_reset_device(input_dev);
>
> We still need to restore LED state after resume. Does the patch below
> work for you?
Finally found some time to test the patch! Not much left of the initial
one though. :)
The patch works for our use-case, i.e. wake-up the phone by pressing a
key and then be able to retrieve what key was pressed.
Please note that I haven't tested the LED/sound and hibernation parts
of the patch.
Thanks,
Oskar
>
> Input: don't call input_dev_release_keys() in resume
>
> From: Aleksej Makarov
>
> When waking up the platform by pressing a specific key, sending a
> release on that key makes it impossible to react on the event in
> user-space. This is fixed by moving the input_reset_device() call to
> resume instead.
>
> [dmitry.torok...@gmail.com: make sure we still restore LED/sound state
> after resume, handle hibernation properly]
>
> Signed-off-by: Aleksej Makarov
> Signed-off-by: Oskar Andero
> Signed-off-by: Dmitry Torokhov
> ---
> drivers/input/input.c | 76
> +
> 1 file changed, 57 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 846ccdd..692435a 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -1653,35 +1653,36 @@ static void input_dev_toggle(struct input_dev *dev,
> bool activate)
> */
> void input_reset_device(struct input_dev *dev)
> {
> - mutex_lock(>mutex);
> + unsigned long flags;
>
> - if (dev->users) {
> - input_dev_toggle(dev, true);
> + mutex_lock(>mutex);
> + spin_lock_irqsave(>event_lock, flags);
>
> - /*
> - * Keys that have been pressed at suspend time are unlikely
> - * to be still pressed when we resume.
> - */
> - spin_lock_irq(>event_lock);
> - input_dev_release_keys(dev);
> - spin_unlock_irq(>event_lock);
> - }
> + input_dev_toggle(dev, true);
> + input_dev_release_keys(dev);
>
> + spin_unlock_irqrestore(>event_lock, flags);
> mutex_unlock(>mutex);
> }
> EXPORT_SYMBOL(input_reset_device);
>
> -#ifdef CONFIG_PM
> +#ifdef CONFIG_PM_SLEEP
> static int input_dev_suspend(struct device *dev)
> {
> struct input_dev *input_dev = to_input_dev(dev);
>
> - mutex_lock(_dev->mutex);
> + spin_lock_irq(_dev->event_lock);
>
> - if (input_dev->users)
> - input_dev_toggle(input_dev, false);
> + /*
> + * Keys that are pressed now are unlikely to be
> + * still pressed when we resume.
> + */
> + input_dev_release_keys(input_dev);
>
> - mutex_unlock(_dev->mutex);
> + /* Turn off LEDs and sounds, if any are active. */
> + input_dev_toggle(input_dev, false);
> +
> + spin_unlock_irq(_dev->event_lock);
>
> return 0;
> }
> @@ -1690,7 +1691,43 @@ static int input_dev_resume(struct device *dev)
> {
> struct input_dev *input_dev = to_input_dev(dev);
>
> - input_reset_device(input_dev);
> + spin_lock_irq(_dev->event_lock);
> +
> + /* Restor
Re: [PATCH v2] input: don't call input_dev_release_keys() in resume
Hi Dmitry,
On 03:14 Tue 26 Nov , Dmitry Torokhov wrote:
Hi Oskar,
On Fri, Nov 22, 2013 at 02:27:04PM +0100, Oskar Andero wrote:
From: Aleksej Makarov aleksej.maka...@sonymobile.com
When waking up the platform by pressing a specific key, sending a
release on that key makes it impossible to react on the event in
user-space. This is fixed by moving the input_reset_device() call to
resume instead.
Cc: Dmitry Torokhov dmitry.torok...@gmail.com
Reviewed-by: Radovan Lekanovic radovan.lekano...@sonymobile.com
Signed-off-by: Aleksej Makarov aleksej.maka...@sonymobile.com
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
---
drivers/input/input.c | 11 +--
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 846ccdd..511d490 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1676,22 +1676,13 @@ static int input_dev_suspend(struct device *dev)
{
struct input_dev *input_dev = to_input_dev(dev);
- mutex_lock(input_dev-mutex);
-
- if (input_dev-users)
- input_dev_toggle(input_dev, false);
-
- mutex_unlock(input_dev-mutex);
+ input_reset_device(input_dev);
return 0;
}
static int input_dev_resume(struct device *dev)
{
- struct input_dev *input_dev = to_input_dev(dev);
-
- input_reset_device(input_dev);
We still need to restore LED state after resume. Does the patch below
work for you?
Finally found some time to test the patch! Not much left of the initial
one though. :)
The patch works for our use-case, i.e. wake-up the phone by pressing a
key and then be able to retrieve what key was pressed.
Please note that I haven't tested the LED/sound and hibernation parts
of the patch.
Thanks,
Oskar
Input: don't call input_dev_release_keys() in resume
From: Aleksej Makarov aleksej.maka...@sonymobile.com
When waking up the platform by pressing a specific key, sending a
release on that key makes it impossible to react on the event in
user-space. This is fixed by moving the input_reset_device() call to
resume instead.
[dmitry.torok...@gmail.com: make sure we still restore LED/sound state
after resume, handle hibernation properly]
Signed-off-by: Aleksej Makarov aleksej.maka...@sonymobile.com
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
Signed-off-by: Dmitry Torokhov dmitry.torok...@gmail.com
---
drivers/input/input.c | 76
+
1 file changed, 57 insertions(+), 19 deletions(-)
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 846ccdd..692435a 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1653,35 +1653,36 @@ static void input_dev_toggle(struct input_dev *dev,
bool activate)
*/
void input_reset_device(struct input_dev *dev)
{
- mutex_lock(dev-mutex);
+ unsigned long flags;
- if (dev-users) {
- input_dev_toggle(dev, true);
+ mutex_lock(dev-mutex);
+ spin_lock_irqsave(dev-event_lock, flags);
- /*
- * Keys that have been pressed at suspend time are unlikely
- * to be still pressed when we resume.
- */
- spin_lock_irq(dev-event_lock);
- input_dev_release_keys(dev);
- spin_unlock_irq(dev-event_lock);
- }
+ input_dev_toggle(dev, true);
+ input_dev_release_keys(dev);
+ spin_unlock_irqrestore(dev-event_lock, flags);
mutex_unlock(dev-mutex);
}
EXPORT_SYMBOL(input_reset_device);
-#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
static int input_dev_suspend(struct device *dev)
{
struct input_dev *input_dev = to_input_dev(dev);
- mutex_lock(input_dev-mutex);
+ spin_lock_irq(input_dev-event_lock);
- if (input_dev-users)
- input_dev_toggle(input_dev, false);
+ /*
+ * Keys that are pressed now are unlikely to be
+ * still pressed when we resume.
+ */
+ input_dev_release_keys(input_dev);
- mutex_unlock(input_dev-mutex);
+ /* Turn off LEDs and sounds, if any are active. */
+ input_dev_toggle(input_dev, false);
+
+ spin_unlock_irq(input_dev-event_lock);
return 0;
}
@@ -1690,7 +1691,43 @@ static int input_dev_resume(struct device *dev)
{
struct input_dev *input_dev = to_input_dev(dev);
- input_reset_device(input_dev);
+ spin_lock_irq(input_dev-event_lock);
+
+ /* Restore state of LEDs and sounds, if any were active. */
+ input_dev_toggle(input_dev, true);
+
+ spin_unlock_irq(input_dev-event_lock);
+
+ return 0;
+}
+
+static int input_dev_freeze(struct device *dev)
+{
+ struct input_dev *input_dev = to_input_dev(dev);
+
+ spin_lock_irq(input_dev-event_lock);
+
+ /*
+ * Keys that are pressed now are unlikely to be
+ * still pressed when we resume
[PATCH v2] input: don't call input_dev_release_keys() in resume
From: Aleksej Makarov
When waking up the platform by pressing a specific key, sending a
release on that key makes it impossible to react on the event in
user-space. This is fixed by moving the input_reset_device() call to
resume instead.
Cc: Dmitry Torokhov
Reviewed-by: Radovan Lekanovic
Signed-off-by: Aleksej Makarov
Signed-off-by: Oskar Andero
---
drivers/input/input.c | 11 +--
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 846ccdd..511d490 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1676,22 +1676,13 @@ static int input_dev_suspend(struct device *dev)
{
struct input_dev *input_dev = to_input_dev(dev);
- mutex_lock(_dev->mutex);
-
- if (input_dev->users)
- input_dev_toggle(input_dev, false);
-
- mutex_unlock(_dev->mutex);
+ input_reset_device(input_dev);
return 0;
}
static int input_dev_resume(struct device *dev)
{
- struct input_dev *input_dev = to_input_dev(dev);
-
- input_reset_device(input_dev);
-
return 0;
}
--
1.8.1.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 v2] input: don't call input_dev_release_keys() in resume
From: Aleksej Makarov aleksej.maka...@sonymobile.com
When waking up the platform by pressing a specific key, sending a
release on that key makes it impossible to react on the event in
user-space. This is fixed by moving the input_reset_device() call to
resume instead.
Cc: Dmitry Torokhov dmitry.torok...@gmail.com
Reviewed-by: Radovan Lekanovic radovan.lekano...@sonymobile.com
Signed-off-by: Aleksej Makarov aleksej.maka...@sonymobile.com
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
---
drivers/input/input.c | 11 +--
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 846ccdd..511d490 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1676,22 +1676,13 @@ static int input_dev_suspend(struct device *dev)
{
struct input_dev *input_dev = to_input_dev(dev);
- mutex_lock(input_dev-mutex);
-
- if (input_dev-users)
- input_dev_toggle(input_dev, false);
-
- mutex_unlock(input_dev-mutex);
+ input_reset_device(input_dev);
return 0;
}
static int input_dev_resume(struct device *dev)
{
- struct input_dev *input_dev = to_input_dev(dev);
-
- input_reset_device(input_dev);
-
return 0;
}
--
1.8.1.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 1/1] MMC: Detect execution mode errors after r/w command
Hi Ulf,
On 18:50 Tue 22 Oct , Ulf Hansson wrote:
> > And this is after the patch has been applied:
> > KB reclen write rewritereadreread
> >51200 4 251 990 3280 3244
> >51200 8 4601545 4460 4463
> >51200 16 8782633 7023 7028
> >51200 3213804394 9802 9832
> >51200 64245762161231412314
> >51200 128366778941408714088
> >51200 256642259161508515086
> >51200 5125536 109941257115659
> >512001024911294991620316205
> >512002048 10197 105021636316368
> >512004096 10524 10238 885016309
> >5120081929615 104561652816529
> >51200 16384 10553 104281680316803
>
> Hi Oskar,
>
> The numbers were not that impressing from the beginning, so that could
> be why you don't see any impact.
> What kind of card are you using, eMMC/SD? In what speed mode are the
> card operating in?
The test was run on an Beagleboard with sdcard. I will try to find another
board with
eMMC support and rerun the tests.
> >> > + /*
> >> > +* Try to get card status which indicates the card state after
> >> > +* command execution. If the first attempt fails, we can't be
> >> > +* sure the returned status is for the r/w command.
> >> > +*/
> >> > + for (retries = 2; retries >= 0; retries--) {
> >> > + err = get_card_status(card, , 0);
> >> > + if (!err)
> >> > + break;
> >> > +
> >> > + status_valid = false;
> >> > + pr_err("%s: error %d sending status command, %sing\n",
> >> > + req->rq_disk->disk_name, err,
> >> > + retries ? "retry" : "abort");
> >> > + }
> >>
> >> Do we have to issue a CMD13 (get_card_status), even if we are using
> >> the open-ended transmission sequence? In other words, could we make
> >> use of the response from CMD12 (stop transmission) instead?
> >
> > That's probably a good idea. Do you know of a way to check if CMD12 has been
> > sent or not?
>
> Have a look for "mmc_host_cmd23", which gets translated into
> "MMC_BLK_CMD23" for the mmc block layer. This will give you some hints
> of were to look.
Thanks! We will revise the patch according to your comments.
-Oskar
--
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 1/1] MMC: Detect execution mode errors after r/w command
Hi Ulf,
On 18:50 Tue 22 Oct , Ulf Hansson wrote:
And this is after the patch has been applied:
KB reclen write rewritereadreread
51200 4 251 990 3280 3244
51200 8 4601545 4460 4463
51200 16 8782633 7023 7028
51200 3213804394 9802 9832
51200 64245762161231412314
51200 128366778941408714088
51200 256642259161508515086
51200 5125536 109941257115659
512001024911294991620316205
512002048 10197 105021636316368
512004096 10524 10238 885016309
5120081929615 104561652816529
51200 16384 10553 104281680316803
Hi Oskar,
The numbers were not that impressing from the beginning, so that could
be why you don't see any impact.
What kind of card are you using, eMMC/SD? In what speed mode are the
card operating in?
The test was run on an Beagleboard with sdcard. I will try to find another
board with
eMMC support and rerun the tests.
+ /*
+* Try to get card status which indicates the card state after
+* command execution. If the first attempt fails, we can't be
+* sure the returned status is for the r/w command.
+*/
+ for (retries = 2; retries = 0; retries--) {
+ err = get_card_status(card, status, 0);
+ if (!err)
+ break;
+
+ status_valid = false;
+ pr_err(%s: error %d sending status command, %sing\n,
+ req-rq_disk-disk_name, err,
+ retries ? retry : abort);
+ }
Do we have to issue a CMD13 (get_card_status), even if we are using
the open-ended transmission sequence? In other words, could we make
use of the response from CMD12 (stop transmission) instead?
That's probably a good idea. Do you know of a way to check if CMD12 has been
sent or not?
Have a look for mmc_host_cmd23, which gets translated into
MMC_BLK_CMD23 for the mmc block layer. This will give you some hints
of were to look.
Thanks! We will revise the patch according to your comments.
-Oskar
--
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 1/1] MMC: Detect execution mode errors after r/w command
Hi Ulf, On 17:01 Wed 16 Oct , Ulf Hansson wrote: > Hi Oskar / Lars, > > Sorry for the delayed response! > > On 10 October 2013 15:28, Oskar Andero wrote: > > From: Lars Svensson > > > > Some error bits in the status field of R1/R1b response are only set > > by the device in response to the command following the failing > > command. The status is only read and checked after a r/w command if > > an error is detected during the initial command or the following data > > transfer. In some situations this causes errors passing undetected. > > > > The solution is to read the status and check for these errors after > > each r/w operation. > > I am a bit concerned about performance, especially when operating on > small packets. > > Previously we already sent a CMD13 after each write, thus this change > will have no effect on write performance. But for read, this will add > a CMD13 check after each request. Have you made any performance > measurement - how big is the impact? It is certainly interested to > know before proceeding. I just ran some iozone tests and I don't see any dramatic degrade in performance. This is before applying the patch: Command line used: ./iozone_arm -az -i0 -i1 -s 50m -I Output is in Kbytes/sec Time Resolution = 0.30 seconds. Processor cache size set to 1024 Kbytes. Processor cache line size set to 32 bytes. File stride size set to 17 * record size. KB reclen write rewritereadreread 51200 4 240 970 3294 3299 51200 8 4571575 4648 4549 51200 16 8622366 6487 6332 51200 3214894181 8642 8661 51200 64250959281085210855 51200 128371378561273812742 51200 256616790041440414410 51200 5128299 104481541615414 512001024933794581607916082 512002048922297291636116365 5120040968926 105261648516477 512008192 10035 10179 855016455 51200 16384 10286 107261683416835 And this is after the patch has been applied: KB reclen write rewritereadreread 51200 4 251 990 3280 3244 51200 8 4601545 4460 4463 51200 16 8782633 7023 7028 51200 3213804394 9802 9832 51200 64245762161231412314 51200 128366778941408714088 51200 256642259161508515086 51200 5125536 109941257115659 512001024911294991620316205 512002048 10197 105021636316368 512004096 10524 10238 885016309 5120081929615 104561652816529 51200 16384 10553 104281680316803 > > > > Signed-off-by: Lars Svensson > > Signed-off-by: Oskar Andero > > Cc: linux-...@vger.kernel.org > > --- > > drivers/mmc/card/block.c | 105 > > +-- > > 1 file changed, 57 insertions(+), 48 deletions(-) > > > > diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c > > index 1a3163f..05de087 100644 > > --- a/drivers/mmc/card/block.c > > +++ b/drivers/mmc/card/block.c > > @@ -797,10 +797,9 @@ static int mmc_blk_cmd_error(struct request *req, > > const char *name, int error, > > * Initial r/w and stop cmd error recovery. > > * We don't know whether the card received the r/w cmd or not, so try to > > * restore things back to a sane state. Essentially, we do this as > > follows: > > - * - Obtain card status. If the first attempt to obtain card status fails, > > - * the status word will reflect the failed status cmd, not the failed > > - * r/w cmd. If we fail to obtain card status, it suggests we can no > > - * longer communicate with the card. > > + * - Check card status. If the status_valid argument is false, the first > > attempt > > + * to obtain card status failed and the status argument will not reflect > > the > > + * failed r/w cmd. > > * - Check the card state. If the card received the cmd but there was a > > * transient problem with the response, it might still be in a data > > transfer > > * mode. Try to send it a stop command. If this fails, we can't > > recover. > > @@ -812,38 +811,15 @
Re: [PATCH 1/1] MMC: Detect execution mode errors after r/w command
Hi Ulf, On 17:01 Wed 16 Oct , Ulf Hansson wrote: Hi Oskar / Lars, Sorry for the delayed response! On 10 October 2013 15:28, Oskar Andero oskar.and...@sonymobile.com wrote: From: Lars Svensson lars1.svens...@sonymobile.com Some error bits in the status field of R1/R1b response are only set by the device in response to the command following the failing command. The status is only read and checked after a r/w command if an error is detected during the initial command or the following data transfer. In some situations this causes errors passing undetected. The solution is to read the status and check for these errors after each r/w operation. I am a bit concerned about performance, especially when operating on small packets. Previously we already sent a CMD13 after each write, thus this change will have no effect on write performance. But for read, this will add a CMD13 check after each request. Have you made any performance measurement - how big is the impact? It is certainly interested to know before proceeding. I just ran some iozone tests and I don't see any dramatic degrade in performance. This is before applying the patch: Command line used: ./iozone_arm -az -i0 -i1 -s 50m -I Output is in Kbytes/sec Time Resolution = 0.30 seconds. Processor cache size set to 1024 Kbytes. Processor cache line size set to 32 bytes. File stride size set to 17 * record size. KB reclen write rewritereadreread 51200 4 240 970 3294 3299 51200 8 4571575 4648 4549 51200 16 8622366 6487 6332 51200 3214894181 8642 8661 51200 64250959281085210855 51200 128371378561273812742 51200 256616790041440414410 51200 5128299 104481541615414 512001024933794581607916082 512002048922297291636116365 5120040968926 105261648516477 512008192 10035 10179 855016455 51200 16384 10286 107261683416835 And this is after the patch has been applied: KB reclen write rewritereadreread 51200 4 251 990 3280 3244 51200 8 4601545 4460 4463 51200 16 8782633 7023 7028 51200 3213804394 9802 9832 51200 64245762161231412314 51200 128366778941408714088 51200 256642259161508515086 51200 5125536 109941257115659 512001024911294991620316205 512002048 10197 105021636316368 512004096 10524 10238 885016309 5120081929615 104561652816529 51200 16384 10553 104281680316803 Signed-off-by: Lars Svensson lars1.svens...@sonymobile.com Signed-off-by: Oskar Andero oskar.and...@sonymobile.com Cc: linux-...@vger.kernel.org --- drivers/mmc/card/block.c | 105 +-- 1 file changed, 57 insertions(+), 48 deletions(-) diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c index 1a3163f..05de087 100644 --- a/drivers/mmc/card/block.c +++ b/drivers/mmc/card/block.c @@ -797,10 +797,9 @@ static int mmc_blk_cmd_error(struct request *req, const char *name, int error, * Initial r/w and stop cmd error recovery. * We don't know whether the card received the r/w cmd or not, so try to * restore things back to a sane state. Essentially, we do this as follows: - * - Obtain card status. If the first attempt to obtain card status fails, - * the status word will reflect the failed status cmd, not the failed - * r/w cmd. If we fail to obtain card status, it suggests we can no - * longer communicate with the card. + * - Check card status. If the status_valid argument is false, the first attempt + * to obtain card status failed and the status argument will not reflect the + * failed r/w cmd. * - Check the card state. If the card received the cmd but there was a * transient problem with the response, it might still be in a data transfer * mode. Try to send it a stop command. If this fails, we can't recover. @@ -812,38 +811,15 @@ static int mmc_blk_cmd_error(struct request *req, const char *name, int error, * Otherwise we don't understand what happened, so abort. */ static int mmc_blk_cmd_recovery(struct mmc_card *card, struct request *req, - struct mmc_blk_request *brq, int *ecc_err, int *gen_err) + struct
[PATCH 1/1] MMC: Detect execution mode errors after r/w command
From: Lars Svensson
Some error bits in the status field of R1/R1b response are only set
by the device in response to the command following the failing
command. The status is only read and checked after a r/w command if
an error is detected during the initial command or the following data
transfer. In some situations this causes errors passing undetected.
The solution is to read the status and check for these errors after
each r/w operation.
Signed-off-by: Lars Svensson
Signed-off-by: Oskar Andero
Cc: linux-...@vger.kernel.org
---
drivers/mmc/card/block.c | 105 +--
1 file changed, 57 insertions(+), 48 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 1a3163f..05de087 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -797,10 +797,9 @@ static int mmc_blk_cmd_error(struct request *req, const
char *name, int error,
* Initial r/w and stop cmd error recovery.
* We don't know whether the card received the r/w cmd or not, so try to
* restore things back to a sane state. Essentially, we do this as follows:
- * - Obtain card status. If the first attempt to obtain card status fails,
- * the status word will reflect the failed status cmd, not the failed
- * r/w cmd. If we fail to obtain card status, it suggests we can no
- * longer communicate with the card.
+ * - Check card status. If the status_valid argument is false, the first
attempt
+ * to obtain card status failed and the status argument will not reflect the
+ * failed r/w cmd.
* - Check the card state. If the card received the cmd but there was a
* transient problem with the response, it might still be in a data transfer
* mode. Try to send it a stop command. If this fails, we can't recover.
@@ -812,38 +811,15 @@ static int mmc_blk_cmd_error(struct request *req, const
char *name, int error,
* Otherwise we don't understand what happened, so abort.
*/
static int mmc_blk_cmd_recovery(struct mmc_card *card, struct request *req,
- struct mmc_blk_request *brq, int *ecc_err, int *gen_err)
+ struct mmc_blk_request *brq, int *ecc_err, int *gen_err,
+ bool status_valid, int status)
{
- bool prev_cmd_status_valid = true;
- u32 status, stop_status = 0;
- int err, retry;
+ u32 stop_status = 0;
+ int err;
if (mmc_card_removed(card))
return ERR_NOMEDIUM;
- /*
-* Try to get card status which indicates both the card state
-* and why there was no response. If the first attempt fails,
-* we can't be sure the returned status is for the r/w command.
-*/
- for (retry = 2; retry >= 0; retry--) {
- err = get_card_status(card, , 0);
- if (!err)
- break;
-
- prev_cmd_status_valid = false;
- pr_err("%s: error %d sending status command, %sing\n",
- req->rq_disk->disk_name, err, retry ? "retry" : "abort");
- }
-
- /* We couldn't get a response from the card. Give up. */
- if (err) {
- /* Check if the card is removed */
- if (mmc_detect_card_removed(card->host))
- return ERR_NOMEDIUM;
- return ERR_ABORT;
- }
-
/* Flag ECC errors */
if ((status & R1_CARD_ECC_FAILED) ||
(brq->stop.resp[0] & R1_CARD_ECC_FAILED) ||
@@ -891,12 +867,12 @@ static int mmc_blk_cmd_recovery(struct mmc_card *card,
struct request *req,
/* Check for set block count errors */
if (brq->sbc.error)
return mmc_blk_cmd_error(req, "SET_BLOCK_COUNT", brq->sbc.error,
- prev_cmd_status_valid, status);
+ status_valid, status);
/* Check for r/w command errors */
if (brq->cmd.error)
return mmc_blk_cmd_error(req, "r/w cmd", brq->cmd.error,
- prev_cmd_status_valid, status);
+ status_valid, status);
/* Data errors */
if (!brq->stop.error)
@@ -1107,6 +1083,12 @@ static inline void mmc_apply_rel_rw(struct
mmc_blk_request *brq,
R1_CC_ERROR | /* Card controller error */ \
R1_ERROR) /* General/unknown error */
+#define EXE_ERRORS \
+ (R1_OUT_OF_RANGE | /* Command argument out of range */ \
+R1_ADDRESS_ERROR | /* Misaligned address */\
+R1_WP_VIOLATION | /* Tried to write to protected block */ \
+R1_ERROR) /* General/unknown error */
+
static int mmc_blk_err_check(struct mmc_card *card,
struct mmc_async_req *areq)
{
@@ -1114,7 +1096,33 @@ static int mmc_blk_err_che
[PATCH 1/1] MMC: Detect execution mode errors after r/w command
From: Lars Svensson lars1.svens...@sonymobile.com
Some error bits in the status field of R1/R1b response are only set
by the device in response to the command following the failing
command. The status is only read and checked after a r/w command if
an error is detected during the initial command or the following data
transfer. In some situations this causes errors passing undetected.
The solution is to read the status and check for these errors after
each r/w operation.
Signed-off-by: Lars Svensson lars1.svens...@sonymobile.com
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
Cc: linux-...@vger.kernel.org
---
drivers/mmc/card/block.c | 105 +--
1 file changed, 57 insertions(+), 48 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 1a3163f..05de087 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -797,10 +797,9 @@ static int mmc_blk_cmd_error(struct request *req, const
char *name, int error,
* Initial r/w and stop cmd error recovery.
* We don't know whether the card received the r/w cmd or not, so try to
* restore things back to a sane state. Essentially, we do this as follows:
- * - Obtain card status. If the first attempt to obtain card status fails,
- * the status word will reflect the failed status cmd, not the failed
- * r/w cmd. If we fail to obtain card status, it suggests we can no
- * longer communicate with the card.
+ * - Check card status. If the status_valid argument is false, the first
attempt
+ * to obtain card status failed and the status argument will not reflect the
+ * failed r/w cmd.
* - Check the card state. If the card received the cmd but there was a
* transient problem with the response, it might still be in a data transfer
* mode. Try to send it a stop command. If this fails, we can't recover.
@@ -812,38 +811,15 @@ static int mmc_blk_cmd_error(struct request *req, const
char *name, int error,
* Otherwise we don't understand what happened, so abort.
*/
static int mmc_blk_cmd_recovery(struct mmc_card *card, struct request *req,
- struct mmc_blk_request *brq, int *ecc_err, int *gen_err)
+ struct mmc_blk_request *brq, int *ecc_err, int *gen_err,
+ bool status_valid, int status)
{
- bool prev_cmd_status_valid = true;
- u32 status, stop_status = 0;
- int err, retry;
+ u32 stop_status = 0;
+ int err;
if (mmc_card_removed(card))
return ERR_NOMEDIUM;
- /*
-* Try to get card status which indicates both the card state
-* and why there was no response. If the first attempt fails,
-* we can't be sure the returned status is for the r/w command.
-*/
- for (retry = 2; retry = 0; retry--) {
- err = get_card_status(card, status, 0);
- if (!err)
- break;
-
- prev_cmd_status_valid = false;
- pr_err(%s: error %d sending status command, %sing\n,
- req-rq_disk-disk_name, err, retry ? retry : abort);
- }
-
- /* We couldn't get a response from the card. Give up. */
- if (err) {
- /* Check if the card is removed */
- if (mmc_detect_card_removed(card-host))
- return ERR_NOMEDIUM;
- return ERR_ABORT;
- }
-
/* Flag ECC errors */
if ((status R1_CARD_ECC_FAILED) ||
(brq-stop.resp[0] R1_CARD_ECC_FAILED) ||
@@ -891,12 +867,12 @@ static int mmc_blk_cmd_recovery(struct mmc_card *card,
struct request *req,
/* Check for set block count errors */
if (brq-sbc.error)
return mmc_blk_cmd_error(req, SET_BLOCK_COUNT, brq-sbc.error,
- prev_cmd_status_valid, status);
+ status_valid, status);
/* Check for r/w command errors */
if (brq-cmd.error)
return mmc_blk_cmd_error(req, r/w cmd, brq-cmd.error,
- prev_cmd_status_valid, status);
+ status_valid, status);
/* Data errors */
if (!brq-stop.error)
@@ -1107,6 +1083,12 @@ static inline void mmc_apply_rel_rw(struct
mmc_blk_request *brq,
R1_CC_ERROR | /* Card controller error */ \
R1_ERROR) /* General/unknown error */
+#define EXE_ERRORS \
+ (R1_OUT_OF_RANGE | /* Command argument out of range */ \
+R1_ADDRESS_ERROR | /* Misaligned address */\
+R1_WP_VIOLATION | /* Tried to write to protected block */ \
+R1_ERROR) /* General/unknown error */
+
static int mmc_blk_err_check(struct mmc_card *card,
struct mmc_async_req *areq)
{
@@ -1114,7 +1096,33 @@ static int mmc_blk_err_check(struct mmc_card
Re: [PATCH v2] input: don't call input_dev_release_keys() in resume
Hii Dmitry,
On 14:09 Thu 25 Jul , Oskar Andero wrote:
> From: Aleksej Makarov
>
> When waking up the platform by pressing a specific key, sending a
> release on that key makes it impossible to react on the event in
> user-space. This is fixed by moving the input_reset_device() call to
> resume instead.
>
> Cc: Dmitry Torokhov
> Reviewed-by: Radovan Lekanovic
> Signed-off-by: Aleksej Makarov
> Signed-off-by: Oskar Andero
> ---
> drivers/input/input.c | 11 +--
> 1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index c044699..ee3ff16 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -1676,22 +1676,13 @@ static int input_dev_suspend(struct device *dev)
> {
> struct input_dev *input_dev = to_input_dev(dev);
>
> - mutex_lock(_dev->mutex);
> -
> - if (input_dev->users)
> - input_dev_toggle(input_dev, false);
> -
> - mutex_unlock(_dev->mutex);
> + input_reset_device(input_dev);
>
> return 0;
> }
>
> static int input_dev_resume(struct device *dev)
> {
> - struct input_dev *input_dev = to_input_dev(dev);
> -
> - input_reset_device(input_dev);
> -
> return 0;
> }
Sorry for bugging you with this patch again! I realize that changes to
input.c is sensitive since it's a central part of the subsystem.
However, the problem of reading input events after wake-up remains.
Does the patch make sense or do you see any potential risks with it?
Thanks,
Oskar
--
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 v2] input: don't call input_dev_release_keys() in resume
Hii Dmitry,
On 14:09 Thu 25 Jul , Oskar Andero wrote:
From: Aleksej Makarov aleksej.maka...@sonymobile.com
When waking up the platform by pressing a specific key, sending a
release on that key makes it impossible to react on the event in
user-space. This is fixed by moving the input_reset_device() call to
resume instead.
Cc: Dmitry Torokhov dmitry.torok...@gmail.com
Reviewed-by: Radovan Lekanovic radovan.lekano...@sonymobile.com
Signed-off-by: Aleksej Makarov aleksej.maka...@sonymobile.com
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
---
drivers/input/input.c | 11 +--
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/input/input.c b/drivers/input/input.c
index c044699..ee3ff16 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1676,22 +1676,13 @@ static int input_dev_suspend(struct device *dev)
{
struct input_dev *input_dev = to_input_dev(dev);
- mutex_lock(input_dev-mutex);
-
- if (input_dev-users)
- input_dev_toggle(input_dev, false);
-
- mutex_unlock(input_dev-mutex);
+ input_reset_device(input_dev);
return 0;
}
static int input_dev_resume(struct device *dev)
{
- struct input_dev *input_dev = to_input_dev(dev);
-
- input_reset_device(input_dev);
-
return 0;
}
Sorry for bugging you with this patch again! I realize that changes to
input.c is sensitive since it's a central part of the subsystem.
However, the problem of reading input events after wake-up remains.
Does the patch make sense or do you see any potential risks with it?
Thanks,
Oskar
--
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 v2] input: don't call input_dev_release_keys() in resume
From: Aleksej Makarov
When waking up the platform by pressing a specific key, sending a
release on that key makes it impossible to react on the event in
user-space. This is fixed by moving the input_reset_device() call to
resume instead.
Cc: Dmitry Torokhov
Reviewed-by: Radovan Lekanovic
Signed-off-by: Aleksej Makarov
Signed-off-by: Oskar Andero
---
drivers/input/input.c | 11 +--
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/input/input.c b/drivers/input/input.c
index c044699..ee3ff16 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1676,22 +1676,13 @@ static int input_dev_suspend(struct device *dev)
{
struct input_dev *input_dev = to_input_dev(dev);
- mutex_lock(_dev->mutex);
-
- if (input_dev->users)
- input_dev_toggle(input_dev, false);
-
- mutex_unlock(_dev->mutex);
+ input_reset_device(input_dev);
return 0;
}
static int input_dev_resume(struct device *dev)
{
- struct input_dev *input_dev = to_input_dev(dev);
-
- input_reset_device(input_dev);
-
return 0;
}
--
1.8.1.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] MMC: Detect execution mode errors after r/w command
Hi,
On 14:47 Tue 09 Jul , Oskar Andero wrote:
> From: Lars Svensson
>
> Some error bits in the status field of R1/R1b response are only set
> by the device in response to the command following the failing
> command. The status is only read and checked after a r/w command if
> an error is detected during the initial command or the following data
> transfer. In some situations this causes errors passing undetected.
>
> The solution is to read the status and check for these errors after
> each r/w operation.
>
> Signed-off-by: Lars Svensson
> Signed-off-by: Oskar Andero
> Cc: linux-...@vger.kernel.org
> ---
> drivers/mmc/card/block.c | 105
> +--
> 1 file changed, 57 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index dd27b07..b2664d7 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -754,10 +754,9 @@ static int mmc_blk_cmd_error(struct request *req, const
> char *name, int error,
> * Initial r/w and stop cmd error recovery.
> * We don't know whether the card received the r/w cmd or not, so try to
> * restore things back to a sane state. Essentially, we do this as follows:
> - * - Obtain card status. If the first attempt to obtain card status fails,
> - * the status word will reflect the failed status cmd, not the failed
> - * r/w cmd. If we fail to obtain card status, it suggests we can no
> - * longer communicate with the card.
> + * - Check card status. If the status_valid argument is false, the first
> attempt
> + * to obtain card status failed and the status argument will not reflect
> the
> + * failed r/w cmd.
> * - Check the card state. If the card received the cmd but there was a
> * transient problem with the response, it might still be in a data
> transfer
> * mode. Try to send it a stop command. If this fails, we can't recover.
> @@ -769,38 +768,15 @@ static int mmc_blk_cmd_error(struct request *req, const
> char *name, int error,
> * Otherwise we don't understand what happened, so abort.
> */
> static int mmc_blk_cmd_recovery(struct mmc_card *card, struct request *req,
> - struct mmc_blk_request *brq, int *ecc_err)
> + struct mmc_blk_request *brq, int *ecc_err, u32 status,
> + bool status_valid)
> {
> - bool prev_cmd_status_valid = true;
> - u32 status, stop_status = 0;
> - int err, retry;
> + u32 stop_status = 0;
> + int err;
>
> if (mmc_card_removed(card))
> return ERR_NOMEDIUM;
>
> - /*
> - * Try to get card status which indicates both the card state
> - * and why there was no response. If the first attempt fails,
> - * we can't be sure the returned status is for the r/w command.
> - */
> - for (retry = 2; retry >= 0; retry--) {
> - err = get_card_status(card, , 0);
> - if (!err)
> - break;
> -
> - prev_cmd_status_valid = false;
> - pr_err("%s: error %d sending status command, %sing\n",
> -req->rq_disk->disk_name, err, retry ? "retry" : "abort");
> - }
> -
> - /* We couldn't get a response from the card. Give up. */
> - if (err) {
> - /* Check if the card is removed */
> - if (mmc_detect_card_removed(card->host))
> - return ERR_NOMEDIUM;
> - return ERR_ABORT;
> - }
> -
> /* Flag ECC errors */
> if ((status & R1_CARD_ECC_FAILED) ||
> (brq->stop.resp[0] & R1_CARD_ECC_FAILED) ||
> @@ -831,12 +807,12 @@ static int mmc_blk_cmd_recovery(struct mmc_card *card,
> struct request *req,
> /* Check for set block count errors */
> if (brq->sbc.error)
> return mmc_blk_cmd_error(req, "SET_BLOCK_COUNT", brq->sbc.error,
> - prev_cmd_status_valid, status);
> + status_valid, status);
>
> /* Check for r/w command errors */
> if (brq->cmd.error)
> return mmc_blk_cmd_error(req, "r/w cmd", brq->cmd.error,
> - prev_cmd_status_valid, status);
> + status_valid, status);
>
> /* Data errors */
> if (!brq->stop.error)
> @@ -1062,6 +1038,12 @@ static inline void mmc_apply_rel_rw(struct
> mmc_blk_request *brq,
>R1_CC_ERROR | /* Card controller error */ \
>R1_ERROR) /* General/unknown error */
>
> +#define EXE_ERRORS
Re: [PATCH] MMC: Detect execution mode errors after r/w command
Hi,
On 14:47 Tue 09 Jul , Oskar Andero wrote:
From: Lars Svensson lars1.svens...@sonymobile.com
Some error bits in the status field of R1/R1b response are only set
by the device in response to the command following the failing
command. The status is only read and checked after a r/w command if
an error is detected during the initial command or the following data
transfer. In some situations this causes errors passing undetected.
The solution is to read the status and check for these errors after
each r/w operation.
Signed-off-by: Lars Svensson lars1.svens...@sonymobile.com
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
Cc: linux-...@vger.kernel.org
---
drivers/mmc/card/block.c | 105
+--
1 file changed, 57 insertions(+), 48 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index dd27b07..b2664d7 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -754,10 +754,9 @@ static int mmc_blk_cmd_error(struct request *req, const
char *name, int error,
* Initial r/w and stop cmd error recovery.
* We don't know whether the card received the r/w cmd or not, so try to
* restore things back to a sane state. Essentially, we do this as follows:
- * - Obtain card status. If the first attempt to obtain card status fails,
- * the status word will reflect the failed status cmd, not the failed
- * r/w cmd. If we fail to obtain card status, it suggests we can no
- * longer communicate with the card.
+ * - Check card status. If the status_valid argument is false, the first
attempt
+ * to obtain card status failed and the status argument will not reflect
the
+ * failed r/w cmd.
* - Check the card state. If the card received the cmd but there was a
* transient problem with the response, it might still be in a data
transfer
* mode. Try to send it a stop command. If this fails, we can't recover.
@@ -769,38 +768,15 @@ static int mmc_blk_cmd_error(struct request *req, const
char *name, int error,
* Otherwise we don't understand what happened, so abort.
*/
static int mmc_blk_cmd_recovery(struct mmc_card *card, struct request *req,
- struct mmc_blk_request *brq, int *ecc_err)
+ struct mmc_blk_request *brq, int *ecc_err, u32 status,
+ bool status_valid)
{
- bool prev_cmd_status_valid = true;
- u32 status, stop_status = 0;
- int err, retry;
+ u32 stop_status = 0;
+ int err;
if (mmc_card_removed(card))
return ERR_NOMEDIUM;
- /*
- * Try to get card status which indicates both the card state
- * and why there was no response. If the first attempt fails,
- * we can't be sure the returned status is for the r/w command.
- */
- for (retry = 2; retry = 0; retry--) {
- err = get_card_status(card, status, 0);
- if (!err)
- break;
-
- prev_cmd_status_valid = false;
- pr_err(%s: error %d sending status command, %sing\n,
-req-rq_disk-disk_name, err, retry ? retry : abort);
- }
-
- /* We couldn't get a response from the card. Give up. */
- if (err) {
- /* Check if the card is removed */
- if (mmc_detect_card_removed(card-host))
- return ERR_NOMEDIUM;
- return ERR_ABORT;
- }
-
/* Flag ECC errors */
if ((status R1_CARD_ECC_FAILED) ||
(brq-stop.resp[0] R1_CARD_ECC_FAILED) ||
@@ -831,12 +807,12 @@ static int mmc_blk_cmd_recovery(struct mmc_card *card,
struct request *req,
/* Check for set block count errors */
if (brq-sbc.error)
return mmc_blk_cmd_error(req, SET_BLOCK_COUNT, brq-sbc.error,
- prev_cmd_status_valid, status);
+ status_valid, status);
/* Check for r/w command errors */
if (brq-cmd.error)
return mmc_blk_cmd_error(req, r/w cmd, brq-cmd.error,
- prev_cmd_status_valid, status);
+ status_valid, status);
/* Data errors */
if (!brq-stop.error)
@@ -1062,6 +1038,12 @@ static inline void mmc_apply_rel_rw(struct
mmc_blk_request *brq,
R1_CC_ERROR | /* Card controller error */ \
R1_ERROR) /* General/unknown error */
+#define EXE_ERRORS \
+ (R1_OUT_OF_RANGE | /* Command argument out of range */ \
+ R1_ADDRESS_ERROR | /* Misaligned address */\
+ R1_WP_VIOLATION | /* Tried to write to protected block */ \
+ R1_ERROR) /* General/unknown error */
+
static int mmc_blk_err_check(struct mmc_card *card,
struct mmc_async_req *areq)
{
@@ -1069,7 +1051,33 @@ static
[PATCH v2] input: don't call input_dev_release_keys() in resume
From: Aleksej Makarov aleksej.maka...@sonymobile.com
When waking up the platform by pressing a specific key, sending a
release on that key makes it impossible to react on the event in
user-space. This is fixed by moving the input_reset_device() call to
resume instead.
Cc: Dmitry Torokhov dmitry.torok...@gmail.com
Reviewed-by: Radovan Lekanovic radovan.lekano...@sonymobile.com
Signed-off-by: Aleksej Makarov aleksej.maka...@sonymobile.com
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
---
drivers/input/input.c | 11 +--
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/input/input.c b/drivers/input/input.c
index c044699..ee3ff16 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1676,22 +1676,13 @@ static int input_dev_suspend(struct device *dev)
{
struct input_dev *input_dev = to_input_dev(dev);
- mutex_lock(input_dev-mutex);
-
- if (input_dev-users)
- input_dev_toggle(input_dev, false);
-
- mutex_unlock(input_dev-mutex);
+ input_reset_device(input_dev);
return 0;
}
static int input_dev_resume(struct device *dev)
{
- struct input_dev *input_dev = to_input_dev(dev);
-
- input_reset_device(input_dev);
-
return 0;
}
--
1.8.1.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] input: don't call input_dev_release_keys() in resume
On 09:46 Fri 05 Jul , Oskar Andero wrote:
> Hi Dmitry,
>
> On 18:33 Thu 04 Apr , Dmitry Torokhov wrote:
> > Hi Oskar,
> >
> > On Thu, Mar 07, 2013 at 03:01:22PM +0100, oskar.and...@sonymobile.com wrote:
> > > From: Aleksej Makarov
> > >
> > > When waking up the platform by pressing a specific key, sending a
> > > release on that key makes it impossible to react on the event in
> > > user-space.
> > >
> >
> > No, we can not simply not release keys after resume from suspend, as
> > this leads to keys being stuck. Consider you are holding an 'I' key on
> > your external USB keyboard and close your laptop's lid. Then you release
> > the key and leave. Later you come back, open the lid waking the laptop
> > and observe endless stream of 'I' in your open terminal.
> >
> > Maybe we should release the keys during suspend time? I am not sure how
> > Android infrastructure will react to this though...
>
> I finally got the time to try this out. Releasing the keys in suspend
> also solves our problem. Would such patch work for the USB keyboard
> case you described? Theoretically, I think it should, right?
>
> So, basically:
>
> static int input_dev_suspend(struct device *dev)
> {
> struct input_dev *input_dev = to_input_dev(dev);
>
> - mutex_lock(_dev->mutex);
> -
> - if (input_dev->users)
> - input_dev_toggle(input_dev, false);
> -
> - mutex_unlock(_dev->mutex);
> + input_reset_device(input_dev);
>
> return 0;
> }
>
> static int input_dev_resume(struct device *dev)
> {
> - struct input_dev *input_dev = to_input_dev(dev);
> -
> - input_reset_device(input_dev);
> -
> return 0;
> }
>
> Should I send the patch?
Ping. Any thoughts on this?
Thanks!
-Oskar
--
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] input: don't call input_dev_release_keys() in resume
On 09:46 Fri 05 Jul , Oskar Andero wrote:
Hi Dmitry,
On 18:33 Thu 04 Apr , Dmitry Torokhov wrote:
Hi Oskar,
On Thu, Mar 07, 2013 at 03:01:22PM +0100, oskar.and...@sonymobile.com wrote:
From: Aleksej Makarov aleksej.maka...@sonymobile.com
When waking up the platform by pressing a specific key, sending a
release on that key makes it impossible to react on the event in
user-space.
No, we can not simply not release keys after resume from suspend, as
this leads to keys being stuck. Consider you are holding an 'I' key on
your external USB keyboard and close your laptop's lid. Then you release
the key and leave. Later you come back, open the lid waking the laptop
and observe endless stream of 'I' in your open terminal.
Maybe we should release the keys during suspend time? I am not sure how
Android infrastructure will react to this though...
I finally got the time to try this out. Releasing the keys in suspend
also solves our problem. Would such patch work for the USB keyboard
case you described? Theoretically, I think it should, right?
So, basically:
static int input_dev_suspend(struct device *dev)
{
struct input_dev *input_dev = to_input_dev(dev);
- mutex_lock(input_dev-mutex);
-
- if (input_dev-users)
- input_dev_toggle(input_dev, false);
-
- mutex_unlock(input_dev-mutex);
+ input_reset_device(input_dev);
return 0;
}
static int input_dev_resume(struct device *dev)
{
- struct input_dev *input_dev = to_input_dev(dev);
-
- input_reset_device(input_dev);
-
return 0;
}
Should I send the patch?
Ping. Any thoughts on this?
Thanks!
-Oskar
--
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] MMC: Detect execution mode errors after r/w command
From: Lars Svensson
Some error bits in the status field of R1/R1b response are only set
by the device in response to the command following the failing
command. The status is only read and checked after a r/w command if
an error is detected during the initial command or the following data
transfer. In some situations this causes errors passing undetected.
The solution is to read the status and check for these errors after
each r/w operation.
Signed-off-by: Lars Svensson
Signed-off-by: Oskar Andero
Cc: linux-...@vger.kernel.org
---
drivers/mmc/card/block.c | 105 +--
1 file changed, 57 insertions(+), 48 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index dd27b07..b2664d7 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -754,10 +754,9 @@ static int mmc_blk_cmd_error(struct request *req, const
char *name, int error,
* Initial r/w and stop cmd error recovery.
* We don't know whether the card received the r/w cmd or not, so try to
* restore things back to a sane state. Essentially, we do this as follows:
- * - Obtain card status. If the first attempt to obtain card status fails,
- * the status word will reflect the failed status cmd, not the failed
- * r/w cmd. If we fail to obtain card status, it suggests we can no
- * longer communicate with the card.
+ * - Check card status. If the status_valid argument is false, the first
attempt
+ * to obtain card status failed and the status argument will not reflect the
+ * failed r/w cmd.
* - Check the card state. If the card received the cmd but there was a
* transient problem with the response, it might still be in a data transfer
* mode. Try to send it a stop command. If this fails, we can't recover.
@@ -769,38 +768,15 @@ static int mmc_blk_cmd_error(struct request *req, const
char *name, int error,
* Otherwise we don't understand what happened, so abort.
*/
static int mmc_blk_cmd_recovery(struct mmc_card *card, struct request *req,
- struct mmc_blk_request *brq, int *ecc_err)
+ struct mmc_blk_request *brq, int *ecc_err, u32 status,
+ bool status_valid)
{
- bool prev_cmd_status_valid = true;
- u32 status, stop_status = 0;
- int err, retry;
+ u32 stop_status = 0;
+ int err;
if (mmc_card_removed(card))
return ERR_NOMEDIUM;
- /*
-* Try to get card status which indicates both the card state
-* and why there was no response. If the first attempt fails,
-* we can't be sure the returned status is for the r/w command.
-*/
- for (retry = 2; retry >= 0; retry--) {
- err = get_card_status(card, , 0);
- if (!err)
- break;
-
- prev_cmd_status_valid = false;
- pr_err("%s: error %d sending status command, %sing\n",
- req->rq_disk->disk_name, err, retry ? "retry" : "abort");
- }
-
- /* We couldn't get a response from the card. Give up. */
- if (err) {
- /* Check if the card is removed */
- if (mmc_detect_card_removed(card->host))
- return ERR_NOMEDIUM;
- return ERR_ABORT;
- }
-
/* Flag ECC errors */
if ((status & R1_CARD_ECC_FAILED) ||
(brq->stop.resp[0] & R1_CARD_ECC_FAILED) ||
@@ -831,12 +807,12 @@ static int mmc_blk_cmd_recovery(struct mmc_card *card,
struct request *req,
/* Check for set block count errors */
if (brq->sbc.error)
return mmc_blk_cmd_error(req, "SET_BLOCK_COUNT", brq->sbc.error,
- prev_cmd_status_valid, status);
+ status_valid, status);
/* Check for r/w command errors */
if (brq->cmd.error)
return mmc_blk_cmd_error(req, "r/w cmd", brq->cmd.error,
- prev_cmd_status_valid, status);
+ status_valid, status);
/* Data errors */
if (!brq->stop.error)
@@ -1062,6 +1038,12 @@ static inline void mmc_apply_rel_rw(struct
mmc_blk_request *brq,
R1_CC_ERROR | /* Card controller error */ \
R1_ERROR) /* General/unknown error */
+#define EXE_ERRORS \
+ (R1_OUT_OF_RANGE | /* Command argument out of range */ \
+R1_ADDRESS_ERROR | /* Misaligned address */\
+R1_WP_VIOLATION | /* Tried to write to protected block */ \
+R1_ERROR) /* General/unknown error */
+
static int mmc_blk_err_check(struct mmc_card *card,
struct mmc_async_req *areq)
{
@@ -1069,7 +1051,33 @@ static int mmc_blk_err_check(struct mmc_car
[PATCH] MMC: Detect execution mode errors after r/w command
From: Lars Svensson lars1.svens...@sonymobile.com
Some error bits in the status field of R1/R1b response are only set
by the device in response to the command following the failing
command. The status is only read and checked after a r/w command if
an error is detected during the initial command or the following data
transfer. In some situations this causes errors passing undetected.
The solution is to read the status and check for these errors after
each r/w operation.
Signed-off-by: Lars Svensson lars1.svens...@sonymobile.com
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
Cc: linux-...@vger.kernel.org
---
drivers/mmc/card/block.c | 105 +--
1 file changed, 57 insertions(+), 48 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index dd27b07..b2664d7 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -754,10 +754,9 @@ static int mmc_blk_cmd_error(struct request *req, const
char *name, int error,
* Initial r/w and stop cmd error recovery.
* We don't know whether the card received the r/w cmd or not, so try to
* restore things back to a sane state. Essentially, we do this as follows:
- * - Obtain card status. If the first attempt to obtain card status fails,
- * the status word will reflect the failed status cmd, not the failed
- * r/w cmd. If we fail to obtain card status, it suggests we can no
- * longer communicate with the card.
+ * - Check card status. If the status_valid argument is false, the first
attempt
+ * to obtain card status failed and the status argument will not reflect the
+ * failed r/w cmd.
* - Check the card state. If the card received the cmd but there was a
* transient problem with the response, it might still be in a data transfer
* mode. Try to send it a stop command. If this fails, we can't recover.
@@ -769,38 +768,15 @@ static int mmc_blk_cmd_error(struct request *req, const
char *name, int error,
* Otherwise we don't understand what happened, so abort.
*/
static int mmc_blk_cmd_recovery(struct mmc_card *card, struct request *req,
- struct mmc_blk_request *brq, int *ecc_err)
+ struct mmc_blk_request *brq, int *ecc_err, u32 status,
+ bool status_valid)
{
- bool prev_cmd_status_valid = true;
- u32 status, stop_status = 0;
- int err, retry;
+ u32 stop_status = 0;
+ int err;
if (mmc_card_removed(card))
return ERR_NOMEDIUM;
- /*
-* Try to get card status which indicates both the card state
-* and why there was no response. If the first attempt fails,
-* we can't be sure the returned status is for the r/w command.
-*/
- for (retry = 2; retry = 0; retry--) {
- err = get_card_status(card, status, 0);
- if (!err)
- break;
-
- prev_cmd_status_valid = false;
- pr_err(%s: error %d sending status command, %sing\n,
- req-rq_disk-disk_name, err, retry ? retry : abort);
- }
-
- /* We couldn't get a response from the card. Give up. */
- if (err) {
- /* Check if the card is removed */
- if (mmc_detect_card_removed(card-host))
- return ERR_NOMEDIUM;
- return ERR_ABORT;
- }
-
/* Flag ECC errors */
if ((status R1_CARD_ECC_FAILED) ||
(brq-stop.resp[0] R1_CARD_ECC_FAILED) ||
@@ -831,12 +807,12 @@ static int mmc_blk_cmd_recovery(struct mmc_card *card,
struct request *req,
/* Check for set block count errors */
if (brq-sbc.error)
return mmc_blk_cmd_error(req, SET_BLOCK_COUNT, brq-sbc.error,
- prev_cmd_status_valid, status);
+ status_valid, status);
/* Check for r/w command errors */
if (brq-cmd.error)
return mmc_blk_cmd_error(req, r/w cmd, brq-cmd.error,
- prev_cmd_status_valid, status);
+ status_valid, status);
/* Data errors */
if (!brq-stop.error)
@@ -1062,6 +1038,12 @@ static inline void mmc_apply_rel_rw(struct
mmc_blk_request *brq,
R1_CC_ERROR | /* Card controller error */ \
R1_ERROR) /* General/unknown error */
+#define EXE_ERRORS \
+ (R1_OUT_OF_RANGE | /* Command argument out of range */ \
+R1_ADDRESS_ERROR | /* Misaligned address */\
+R1_WP_VIOLATION | /* Tried to write to protected block */ \
+R1_ERROR) /* General/unknown error */
+
static int mmc_blk_err_check(struct mmc_card *card,
struct mmc_async_req *areq)
{
@@ -1069,7 +1051,33 @@ static int mmc_blk_err_check(struct mmc_card *card
Re: [PATCH] input: don't call input_dev_release_keys() in resume
Hi Dmitry,
On 18:33 Thu 04 Apr , Dmitry Torokhov wrote:
> Hi Oskar,
>
> On Thu, Mar 07, 2013 at 03:01:22PM +0100, oskar.and...@sonymobile.com wrote:
> > From: Aleksej Makarov
> >
> > When waking up the platform by pressing a specific key, sending a
> > release on that key makes it impossible to react on the event in
> > user-space.
> >
>
> No, we can not simply not release keys after resume from suspend, as
> this leads to keys being stuck. Consider you are holding an 'I' key on
> your external USB keyboard and close your laptop's lid. Then you release
> the key and leave. Later you come back, open the lid waking the laptop
> and observe endless stream of 'I' in your open terminal.
>
> Maybe we should release the keys during suspend time? I am not sure how
> Android infrastructure will react to this though...
I finally got the time to try this out. Releasing the keys in suspend
also solves our problem. Would such patch work for the USB keyboard
case you described? Theoretically, I think it should, right?
So, basically:
static int input_dev_suspend(struct device *dev)
{
struct input_dev *input_dev = to_input_dev(dev);
- mutex_lock(_dev->mutex);
-
- if (input_dev->users)
- input_dev_toggle(input_dev, false);
-
- mutex_unlock(_dev->mutex);
+ input_reset_device(input_dev);
return 0;
}
static int input_dev_resume(struct device *dev)
{
- struct input_dev *input_dev = to_input_dev(dev);
-
- input_reset_device(input_dev);
-
return 0;
}
Should I send the patch?
-Oskar
--
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] input: don't call input_dev_release_keys() in resume
Hi Dmitry,
On 18:33 Thu 04 Apr , Dmitry Torokhov wrote:
Hi Oskar,
On Thu, Mar 07, 2013 at 03:01:22PM +0100, oskar.and...@sonymobile.com wrote:
From: Aleksej Makarov aleksej.maka...@sonymobile.com
When waking up the platform by pressing a specific key, sending a
release on that key makes it impossible to react on the event in
user-space.
No, we can not simply not release keys after resume from suspend, as
this leads to keys being stuck. Consider you are holding an 'I' key on
your external USB keyboard and close your laptop's lid. Then you release
the key and leave. Later you come back, open the lid waking the laptop
and observe endless stream of 'I' in your open terminal.
Maybe we should release the keys during suspend time? I am not sure how
Android infrastructure will react to this though...
I finally got the time to try this out. Releasing the keys in suspend
also solves our problem. Would such patch work for the USB keyboard
case you described? Theoretically, I think it should, right?
So, basically:
static int input_dev_suspend(struct device *dev)
{
struct input_dev *input_dev = to_input_dev(dev);
- mutex_lock(input_dev-mutex);
-
- if (input_dev-users)
- input_dev_toggle(input_dev, false);
-
- mutex_unlock(input_dev-mutex);
+ input_reset_device(input_dev);
return 0;
}
static int input_dev_resume(struct device *dev)
{
- struct input_dev *input_dev = to_input_dev(dev);
-
- input_reset_device(input_dev);
-
return 0;
}
Should I send the patch?
-Oskar
--
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] backlight: lp855x: set zero brightness at FBBLANK
From: Shingo Nakao
When backlight turns on early from display, a white line can be
seen on the screen. Therefore make sure backlight is off when we
are under an fb blank event.
Signed-off-by: Shingo Nakao
Cc: Milo Kim
Cc: Richard Purdie
Signed-off-by: Oskar Andero
---
drivers/video/backlight/lp855x_bl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/backlight/lp855x_bl.c
b/drivers/video/backlight/lp855x_bl.c
index a0e1e02..c0b41f1 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -246,7 +246,7 @@ static int lp855x_bl_update_status(struct backlight_device
*bl)
{
struct lp855x *lp = bl_get_data(bl);
- if (bl->props.state & BL_CORE_SUSPENDED)
+ if (bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
bl->props.brightness = 0;
if (lp->mode == PWM_BASED) {
--
1.8.1.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: storage: Add MicroVault Flash Drive to unusual_devs
From: Ren Bigcren The device report an error capacity when read_capacity_16(). Using read_capacity_10() can get the correct capacity. Signed-off-by: Ren Bigcren Cc: Matthew Dharm Signed-off-by: Oskar Andero --- drivers/usb/storage/unusual_devs.h | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h index 179933528..c015f2c 100644 --- a/drivers/usb/storage/unusual_devs.h +++ b/drivers/usb/storage/unusual_devs.h @@ -665,6 +665,13 @@ UNUSUAL_DEV( 0x054c, 0x016a, 0x, 0x, USB_SC_DEVICE, USB_PR_DEVICE, NULL, US_FL_FIX_INQUIRY ), +/* Submitted by Ren Bigcren */ +UNUSUAL_DEV( 0x054c, 0x02a5, 0x0100, 0x0100, + "Sony Corp.", + "MicroVault Flash Drive", + USB_SC_DEVICE, USB_PR_DEVICE, NULL, + US_FL_NO_READ_CAPACITY_16 ), + /* floppy reports multiple luns */ UNUSUAL_DEV( 0x055d, 0x2020, 0x, 0x0210, "SAMSUNG", -- 1.8.1.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: storage: Add MicroVault Flash Drive to unusual_devs
From: Ren Bigcren bigcren@sonymobile.com The device report an error capacity when read_capacity_16(). Using read_capacity_10() can get the correct capacity. Signed-off-by: Ren Bigcren bigcren@sonymobile.com Cc: Matthew Dharm mdharm-...@one-eyed-alien.net Signed-off-by: Oskar Andero oskar.and...@sonymobile.com --- drivers/usb/storage/unusual_devs.h | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h index 179933528..c015f2c 100644 --- a/drivers/usb/storage/unusual_devs.h +++ b/drivers/usb/storage/unusual_devs.h @@ -665,6 +665,13 @@ UNUSUAL_DEV( 0x054c, 0x016a, 0x, 0x, USB_SC_DEVICE, USB_PR_DEVICE, NULL, US_FL_FIX_INQUIRY ), +/* Submitted by Ren Bigcren bigcren@sonymobile.com */ +UNUSUAL_DEV( 0x054c, 0x02a5, 0x0100, 0x0100, + Sony Corp., + MicroVault Flash Drive, + USB_SC_DEVICE, USB_PR_DEVICE, NULL, + US_FL_NO_READ_CAPACITY_16 ), + /* floppy reports multiple luns */ UNUSUAL_DEV( 0x055d, 0x2020, 0x, 0x0210, SAMSUNG, -- 1.8.1.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] backlight: lp855x: set zero brightness at FBBLANK
From: Shingo Nakao shingo.x.na...@sonymobile.com
When backlight turns on early from display, a white line can be
seen on the screen. Therefore make sure backlight is off when we
are under an fb blank event.
Signed-off-by: Shingo Nakao shingo.x.na...@sonymobile.com
Cc: Milo Kim milo@ti.com
Cc: Richard Purdie rpur...@rpsys.net
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
---
drivers/video/backlight/lp855x_bl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/backlight/lp855x_bl.c
b/drivers/video/backlight/lp855x_bl.c
index a0e1e02..c0b41f1 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -246,7 +246,7 @@ static int lp855x_bl_update_status(struct backlight_device
*bl)
{
struct lp855x *lp = bl_get_data(bl);
- if (bl-props.state BL_CORE_SUSPENDED)
+ if (bl-props.state (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
bl-props.brightness = 0;
if (lp-mode == PWM_BASED) {
--
1.8.1.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 v2] mm: vmscan: add VM_BUG_ON on illegal return values from scan_objects
Add a VM_BUG_ON to catch any illegal value from the shrinkers. It's a potential bug if scan_objects returns a negative other than -1 and would lead to undefined behaviour. Cc: Glauber Costa Cc: Dave Chinner Cc: Andrew Morton Cc: Hugh Dickins Cc: Greg Kroah-Hartman Signed-off-by: Oskar Andero --- mm/vmscan.c | 1 + 1 file changed, 1 insertion(+) diff --git a/mm/vmscan.c b/mm/vmscan.c index 6bac41e..63fec86 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -293,6 +293,7 @@ shrink_slab_one(struct shrinker *shrinker, struct shrink_control *shrinkctl, ret = shrinker->scan_objects(shrinker, shrinkctl); if (ret == -1) break; + VM_BUG_ON(ret < -1); freed += ret; count_vm_events(SLABS_SCANNED, nr_to_scan); -- 1.8.1.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 v2] mm: vmscan: add VM_BUG_ON on illegal return values from scan_objects
Add a VM_BUG_ON to catch any illegal value from the shrinkers. It's a potential bug if scan_objects returns a negative other than -1 and would lead to undefined behaviour. Cc: Glauber Costa glom...@openvz.org Cc: Dave Chinner dchin...@redhat.com Cc: Andrew Morton a...@linux-foundation.org Cc: Hugh Dickins hu...@google.com Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Signed-off-by: Oskar Andero oskar.and...@sonymobile.com --- mm/vmscan.c | 1 + 1 file changed, 1 insertion(+) diff --git a/mm/vmscan.c b/mm/vmscan.c index 6bac41e..63fec86 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -293,6 +293,7 @@ shrink_slab_one(struct shrinker *shrinker, struct shrink_control *shrinkctl, ret = shrinker-scan_objects(shrinker, shrinkctl); if (ret == -1) break; + VM_BUG_ON(ret -1); freed += ret; count_vm_events(SLABS_SCANNED, nr_to_scan); -- 1.8.1.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] mm: vmscan: add BUG_ON on illegal return values from scan_objects
Add a BUG_ON to catch any illegal value from the shrinkers. This fixes a potential bug if scan_objects returns a negative other than -1, which would lead to undefined behaviour. Cc: Glauber Costa Cc: Dave Chinner Cc: Andrew Morton Cc: Hugh Dickins Cc: Greg Kroah-Hartman Signed-off-by: Oskar Andero --- mm/vmscan.c | 1 + 1 file changed, 1 insertion(+) diff --git a/mm/vmscan.c b/mm/vmscan.c index 6bac41e..fbe6742 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -293,6 +293,7 @@ shrink_slab_one(struct shrinker *shrinker, struct shrink_control *shrinkctl, ret = shrinker->scan_objects(shrinker, shrinkctl); if (ret == -1) break; + BUG_ON(ret < -1); freed += ret; count_vm_events(SLABS_SCANNED, nr_to_scan); -- 1.8.1.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] mm: vmscan: add BUG_ON on illegal return values from scan_objects
Add a BUG_ON to catch any illegal value from the shrinkers. This fixes a potential bug if scan_objects returns a negative other than -1, which would lead to undefined behaviour. Cc: Glauber Costa glom...@openvz.org Cc: Dave Chinner dchin...@redhat.com Cc: Andrew Morton a...@linux-foundation.org Cc: Hugh Dickins hu...@google.com Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Signed-off-by: Oskar Andero oskar.and...@sonymobile.com --- mm/vmscan.c | 1 + 1 file changed, 1 insertion(+) diff --git a/mm/vmscan.c b/mm/vmscan.c index 6bac41e..fbe6742 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -293,6 +293,7 @@ shrink_slab_one(struct shrinker *shrinker, struct shrink_control *shrinkctl, ret = shrinker-scan_objects(shrinker, shrinkctl); if (ret == -1) break; + BUG_ON(ret -1); freed += ret; count_vm_events(SLABS_SCANNED, nr_to_scan); -- 1.8.1.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] mm: vmscan: handle any negative return value from scan_objects
On 08:33 Fri 17 May , Dave Chinner wrote: > On Thu, May 16, 2013 at 02:27:52PM +0200, Oskar Andero wrote: > > On 13:52 Thu 16 May , Dave Chinner wrote: > > > On Thu, May 16, 2013 at 10:42:16AM +0200, Oskar Andero wrote: > > > > The shrinkers must return -1 to indicate that it is busy. Instead, treat > > > > any negative value as busy. > > > > > > Why? The API defines return condition for aborting a scan and gives > > > a specific value for doing that. i.e. explain why should change the > > > API to over-specify the 'abort scan" return value like this. > > > > As I pointed out earlier, looking in to the code (from master): > > if (shrink_ret == -1) > > break; > > if (shrink_ret < nr_before) > > ret += nr_before - shrink_ret; > > > > This piece of code lacks a sanity check and will only function if shrink_ret > > is either greater than zero or exactly -1. If shrink_ret is e.g. -2 this > > will > > lead to undefined behaviour. > > If a shrinker is returning -2 then the shrinker is broken and needs > fixing. The point is: returning -2 is just as magical and meaningful as returning -1. Usually, returning a negative means "failure" (Chapter 16 CodingStyle), not a perfectly valid "abort scan" as in this piece of code. > > > FWIW, using "any" negative number for "abort scan" is a bad API > > > design decision. It means that in future we can't introduce > > > different negative return values in the API if we have a new to. > > > i.e. each specific negative return value needs to have the potential > > > for defining a different behaviour. > > > > An alternative to my patch would be to add: > > if (shrink_ret < -1) > >/* handle illegal return code in some way */ > > How? We have one valid negative return code. WTF are we supposed to > do if a shrinker is passing undefined return values? IOWs, the only > sane thing to do is: > > BUG_ON(shrink_ret < -1); Yes, of course! BUG_ON() is the proper way to handle an illegal value. Now we are getting somewhere! > > > So if any change needs to be made, it is to change the -1 return > > > value to an enum and have the shrinkers return that enum when they > > > want an abort. > > > > I am all for an enum, but I still believe we should handle the case where > > the shrinkers return something wicked. > > Which bit of "broken shrinkers need to be fixed" don't you > understand? A BUG_ON() will make sure they get fixed - anything else > that allows broken shrinkers to continue functioning is a completely > unacceptible solution. BUG_ON() is perfect IMO and if everyone is ok with that I will send version 2 of my patch. Now there is just the matter of returning hardcoded -1. Would an enum in shrinker.h add any value? I have gotten different feedback on this - some say yea, others nay. I think I have motivated it enough in this thread, so I am not going to repeat myself. -Oskar -- 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] mm: vmscan: handle any negative return value from scan_objects
On 08:33 Fri 17 May , Dave Chinner wrote: On Thu, May 16, 2013 at 02:27:52PM +0200, Oskar Andero wrote: On 13:52 Thu 16 May , Dave Chinner wrote: On Thu, May 16, 2013 at 10:42:16AM +0200, Oskar Andero wrote: The shrinkers must return -1 to indicate that it is busy. Instead, treat any negative value as busy. Why? The API defines return condition for aborting a scan and gives a specific value for doing that. i.e. explain why should change the API to over-specify the 'abort scan return value like this. As I pointed out earlier, looking in to the code (from master): if (shrink_ret == -1) break; if (shrink_ret nr_before) ret += nr_before - shrink_ret; This piece of code lacks a sanity check and will only function if shrink_ret is either greater than zero or exactly -1. If shrink_ret is e.g. -2 this will lead to undefined behaviour. If a shrinker is returning -2 then the shrinker is broken and needs fixing. The point is: returning -2 is just as magical and meaningful as returning -1. Usually, returning a negative means failure (Chapter 16 CodingStyle), not a perfectly valid abort scan as in this piece of code. FWIW, using any negative number for abort scan is a bad API design decision. It means that in future we can't introduce different negative return values in the API if we have a new to. i.e. each specific negative return value needs to have the potential for defining a different behaviour. An alternative to my patch would be to add: if (shrink_ret -1) /* handle illegal return code in some way */ How? We have one valid negative return code. WTF are we supposed to do if a shrinker is passing undefined return values? IOWs, the only sane thing to do is: BUG_ON(shrink_ret -1); Yes, of course! BUG_ON() is the proper way to handle an illegal value. Now we are getting somewhere! So if any change needs to be made, it is to change the -1 return value to an enum and have the shrinkers return that enum when they want an abort. I am all for an enum, but I still believe we should handle the case where the shrinkers return something wicked. Which bit of broken shrinkers need to be fixed don't you understand? A BUG_ON() will make sure they get fixed - anything else that allows broken shrinkers to continue functioning is a completely unacceptible solution. BUG_ON() is perfect IMO and if everyone is ok with that I will send version 2 of my patch. Now there is just the matter of returning hardcoded -1. Would an enum in shrinker.h add any value? I have gotten different feedback on this - some say yea, others nay. I think I have motivated it enough in this thread, so I am not going to repeat myself. -Oskar -- 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] mm: vmscan: handle any negative return value from scan_objects
On 13:52 Thu 16 May , Dave Chinner wrote: > On Thu, May 16, 2013 at 10:42:16AM +0200, Oskar Andero wrote: > > The shrinkers must return -1 to indicate that it is busy. Instead, treat > > any negative value as busy. > > Why? The API defines return condition for aborting a scan and gives > a specific value for doing that. i.e. explain why should change the > API to over-specify the 'abort scan" return value like this. As I pointed out earlier, looking in to the code (from master): if (shrink_ret == -1) break; if (shrink_ret < nr_before) ret += nr_before - shrink_ret; This piece of code lacks a sanity check and will only function if shrink_ret is either greater than zero or exactly -1. If shrink_ret is e.g. -2 this will lead to undefined behaviour. > FWIW, using "any" negative number for "abort scan" is a bad API > design decision. It means that in future we can't introduce > different negative return values in the API if we have a new to. > i.e. each specific negative return value needs to have the potential > for defining a different behaviour. An alternative to my patch would be to add: if (shrink_ret < -1) /* handle illegal return code in some way */ > So if any change needs to be made, it is to change the -1 return > value to an enum and have the shrinkers return that enum when they > want an abort. I am all for an enum, but I still believe we should handle the case where the shrinkers return something wicked. -Oskar -- 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] mm: vmscan: handle any negative return value from scan_objects
The shrinkers must return -1 to indicate that it is busy. Instead, treat
any negative value as busy.
This fixes a potential bug if scan_objects returns a negative other than -1.
Cc: Glauber Costa
Cc: Dave Chinner
Cc: Andrew Morton
Cc: Hugh Dickins
Cc: Greg Kroah-Hartman
Signed-off-by: Oskar Andero
---
include/linux/shrinker.h | 7 ---
mm/vmscan.c | 2 +-
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
index 3b08869..ced0e91 100644
--- a/include/linux/shrinker.h
+++ b/include/linux/shrinker.h
@@ -38,9 +38,10 @@ struct shrink_control {
* @scan_objects will only be called if @count_objects returned a positive
* value for the number of freeable objects. The callout should scan the cache
* and attempt to free items from the cache. It should then return the number
of
- * objects freed during the scan, or -1 if progress cannot be made due to
- * potential deadlocks. If -1 is returned, then no further attempts to call the
- * @scan_objects will be made from the current reclaim context.
+ * objects freed during the scan, or a negative value if progress cannot be
made
+ * due to potential deadlocks. If a negative value is returned, then no further
+ * attempts to call the @scan_objects will be made from the current reclaim
+ * context.
*/
struct shrinker {
long (*count_objects)(struct shrinker *, struct shrink_control *sc);
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 6bac41e..acb4aef 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -291,7 +291,7 @@ shrink_slab_one(struct shrinker *shrinker, struct
shrink_control *shrinkctl,
shrinkctl->nr_to_scan = nr_to_scan;
ret = shrinker->scan_objects(shrinker, shrinkctl);
- if (ret == -1)
+ if (ret < 0)
break;
freed += ret;
--
1.8.1.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: [RFC PATCH 0/2] return value from shrinkers
On 16:49 Wed 15 May , Glauber Costa wrote: > On 05/15/2013 06:47 PM, Oskar Andero wrote: > > On 16:18 Wed 15 May , Glauber Costa wrote: > >> On 05/15/2013 06:10 PM, Oskar Andero wrote: > >>> On 17:03 Tue 14 May , Glauber Costa wrote: > >>>> On 05/13/2013 06:16 PM, Oskar Andero wrote: > >>>>> Hi, > >>>>> > >>>>> In a previous discussion on lkml it was noted that the shrinkers use the > >>>>> magic value "-1" to signal that something went wrong. > >>>>> > >>>>> This patch-set implements the suggestion of instead using errno.h values > >>>>> to return something more meaningful. > >>>>> > >>>>> The first patch simply changes the check from -1 to any negative value > >>>>> and > >>>>> updates the comment accordingly. > >>>>> > >>>>> The second patch updates the shrinkers to return an errno.h value > >>>>> instead > >>>>> of -1. Since this one spans over many different areas I need input on > >>>>> what is > >>>>> a meaningful return value. Right now I used -EBUSY on everything for > >>>>> consitency. > >>>>> > >>>>> What do you say? Is this a good idea or does it make no sense at all? > >>>>> > >>>>> Thanks! > >>>>> > >>>> > >>>> Right now me and Dave are completely reworking the way shrinkers > >>>> operate. I suggest, first of all, that you take a look at that > >>>> cautiously. > >>> > >>> Sounds good. Where can one find the code for that? > >>> > >> linux-mm, linux-fsdevel > >> > >> Subject is "kmemcg shrinkers", but only the second part is memcg related. > >> > >>>> On the specifics of what you are doing here, what would be the benefit > >>>> of returning something other than -1 ? Is there anything we would do > >>>> differently for a return value lesser than 1? > >>> > >>> Firstly, what bugs me is the magic and unintuitiveness of using -1 rather > >>> than a > >>> more descriptive error code. IMO, even a #define SHRINK_ERROR -1 in some > >>> header > >>> file would be better. > >>> > >>> Expanding the test to <0 will open up for more granular error checks, > >>> like -EAGAIN, -EBUSY and so on. Currently, they would all be treated the > >>> same, > >>> but maybe in the future we would like to handle them differently? > >>> > >> > >> Then in the future we change it. > >> This is not a user visible API, we are free to change it at any time, > >> under any conditions. There is only value in supporting different error > >> codes if we intend to do something different about it. Otherwise, it is > >> just churn. > >> > >> Moreover, -1 does not necessarily mean error. It means "stop shrinking". > >> There are many non-error conditions in which it could happen. > >> > > > > Sure, maybe errno.h is not the right way to go. So, why not add the #define > > instead? E.g. STOP_SHRINKING or something better than -1. > > > >>> Finally, looking at the code: > >>> if (shrink_ret == -1) > >>> break; > >>> if (shrink_ret < nr_before) > >>> ret += nr_before - shrink_ret; > >>> > >>> This piece of code will only function if shrink_ret is either greater > >>> than zero > >>> or -1. If shrink_ret is -2 this will lead to undefined behaviour. > >>> > >> Except it never is. But since we are touching this code anyway, I see no > >> problems in expanding the test. What I don't see the point for, is the > >> other patch in your series in which you return error codes. > >> > >>>> So far, shrink_slab behaves the same, you are just expanding the test. > >>>> If you really want to push this through, I would suggest coming up with > >>>> a more concrete reason for why this is wanted. > >>> > >>> I don't know how well this patch is aligned with your current rework, but > >>> based on my comments above, I don't see a reason for not taking it. > >>> >
Re: [RFC PATCH 0/2] return value from shrinkers
On 01:05 Thu 16 May , Andrew Morton wrote: > On Mon, 13 May 2013 16:16:33 +0200 Oskar Andero > wrote: > > > In a previous discussion on lkml it was noted that the shrinkers use the > > magic value "-1" to signal that something went wrong. > > > > This patch-set implements the suggestion of instead using errno.h values > > to return something more meaningful. > > > > The first patch simply changes the check from -1 to any negative value and > > updates the comment accordingly. > > > > The second patch updates the shrinkers to return an errno.h value instead > > of -1. Since this one spans over many different areas I need input on what > > is > > a meaningful return value. Right now I used -EBUSY on everything for > > consitency. > > > > What do you say? Is this a good idea or does it make no sense at all? > > I don't see much point in it, really. Returning an errno implies that > the errno will eventually be returned to userspace. But that isn't the > case, so such a change is somewhat misleading. Yes. Glauber Costa pointed that out and I agree - errno.h is probably not the right way to go. > If we want the capability to return more than a binary yes/no message > to callers then yes, we could/should enumerate the shrinker return > values. But as that is a different concept from errnos, it should be > done with a different and shrinker-specific namespace. Agreed, but even if there right now is only a binary return message, is a hardcoded -1 considered to be acceptable for an interface? IMHO, it is not very readable nor intuitive for the users of the interface. Why not, as you mention, add a define or enum in shrinker.h instead, e.g. SHRINKER_STOP or something. -Oskar -- 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: [RFC PATCH 0/2] return value from shrinkers
On 01:05 Thu 16 May , Andrew Morton wrote: On Mon, 13 May 2013 16:16:33 +0200 Oskar Andero oskar.and...@sonymobile.com wrote: In a previous discussion on lkml it was noted that the shrinkers use the magic value -1 to signal that something went wrong. This patch-set implements the suggestion of instead using errno.h values to return something more meaningful. The first patch simply changes the check from -1 to any negative value and updates the comment accordingly. The second patch updates the shrinkers to return an errno.h value instead of -1. Since this one spans over many different areas I need input on what is a meaningful return value. Right now I used -EBUSY on everything for consitency. What do you say? Is this a good idea or does it make no sense at all? I don't see much point in it, really. Returning an errno implies that the errno will eventually be returned to userspace. But that isn't the case, so such a change is somewhat misleading. Yes. Glauber Costa pointed that out and I agree - errno.h is probably not the right way to go. If we want the capability to return more than a binary yes/no message to callers then yes, we could/should enumerate the shrinker return values. But as that is a different concept from errnos, it should be done with a different and shrinker-specific namespace. Agreed, but even if there right now is only a binary return message, is a hardcoded -1 considered to be acceptable for an interface? IMHO, it is not very readable nor intuitive for the users of the interface. Why not, as you mention, add a define or enum in shrinker.h instead, e.g. SHRINKER_STOP or something. -Oskar -- 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: [RFC PATCH 0/2] return value from shrinkers
On 16:49 Wed 15 May , Glauber Costa wrote: On 05/15/2013 06:47 PM, Oskar Andero wrote: On 16:18 Wed 15 May , Glauber Costa wrote: On 05/15/2013 06:10 PM, Oskar Andero wrote: On 17:03 Tue 14 May , Glauber Costa wrote: On 05/13/2013 06:16 PM, Oskar Andero wrote: Hi, In a previous discussion on lkml it was noted that the shrinkers use the magic value -1 to signal that something went wrong. This patch-set implements the suggestion of instead using errno.h values to return something more meaningful. The first patch simply changes the check from -1 to any negative value and updates the comment accordingly. The second patch updates the shrinkers to return an errno.h value instead of -1. Since this one spans over many different areas I need input on what is a meaningful return value. Right now I used -EBUSY on everything for consitency. What do you say? Is this a good idea or does it make no sense at all? Thanks! Right now me and Dave are completely reworking the way shrinkers operate. I suggest, first of all, that you take a look at that cautiously. Sounds good. Where can one find the code for that? linux-mm, linux-fsdevel Subject is kmemcg shrinkers, but only the second part is memcg related. On the specifics of what you are doing here, what would be the benefit of returning something other than -1 ? Is there anything we would do differently for a return value lesser than 1? Firstly, what bugs me is the magic and unintuitiveness of using -1 rather than a more descriptive error code. IMO, even a #define SHRINK_ERROR -1 in some header file would be better. Expanding the test to 0 will open up for more granular error checks, like -EAGAIN, -EBUSY and so on. Currently, they would all be treated the same, but maybe in the future we would like to handle them differently? Then in the future we change it. This is not a user visible API, we are free to change it at any time, under any conditions. There is only value in supporting different error codes if we intend to do something different about it. Otherwise, it is just churn. Moreover, -1 does not necessarily mean error. It means stop shrinking. There are many non-error conditions in which it could happen. Sure, maybe errno.h is not the right way to go. So, why not add the #define instead? E.g. STOP_SHRINKING or something better than -1. Finally, looking at the code: if (shrink_ret == -1) break; if (shrink_ret nr_before) ret += nr_before - shrink_ret; This piece of code will only function if shrink_ret is either greater than zero or -1. If shrink_ret is -2 this will lead to undefined behaviour. Except it never is. But since we are touching this code anyway, I see no problems in expanding the test. What I don't see the point for, is the other patch in your series in which you return error codes. So far, shrink_slab behaves the same, you are just expanding the test. If you really want to push this through, I would suggest coming up with a more concrete reason for why this is wanted. I don't know how well this patch is aligned with your current rework, but based on my comments above, I don't see a reason for not taking it. I see no objections for PATCH #1 that expands the check, as a cautionary measure. But I will oppose returning error codes from shrinkers without a solid reason for doing so (meaning a use case in which we really threat one of the errors differently) Sorry for being over-zealous about the return codes and I understand that it is really a minor thing and possibly also a philosophical question. My only solid reasons are unintuiveness and readability. That is how I came across it in the first place. If no-one backs me up on this I will drop the second patch and resend the first patch without RFC prefix. If you are willing to wait a bit until it finally gets merged, please send it against my memcg.git in kernel.org (branch kmemcg-lru-shrinkers). I can carry your patch in our series. Alright. I will apply PATCH 1/2 ontop of your kmemcg-lru-shrinker branch and send it to you offline. Thanks! -Oskar -- 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] mm: vmscan: handle any negative return value from scan_objects
The shrinkers must return -1 to indicate that it is busy. Instead, treat
any negative value as busy.
This fixes a potential bug if scan_objects returns a negative other than -1.
Cc: Glauber Costa glom...@openvz.org
Cc: Dave Chinner dchin...@redhat.com
Cc: Andrew Morton a...@linux-foundation.org
Cc: Hugh Dickins hu...@google.com
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
---
include/linux/shrinker.h | 7 ---
mm/vmscan.c | 2 +-
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
index 3b08869..ced0e91 100644
--- a/include/linux/shrinker.h
+++ b/include/linux/shrinker.h
@@ -38,9 +38,10 @@ struct shrink_control {
* @scan_objects will only be called if @count_objects returned a positive
* value for the number of freeable objects. The callout should scan the cache
* and attempt to free items from the cache. It should then return the number
of
- * objects freed during the scan, or -1 if progress cannot be made due to
- * potential deadlocks. If -1 is returned, then no further attempts to call the
- * @scan_objects will be made from the current reclaim context.
+ * objects freed during the scan, or a negative value if progress cannot be
made
+ * due to potential deadlocks. If a negative value is returned, then no further
+ * attempts to call the @scan_objects will be made from the current reclaim
+ * context.
*/
struct shrinker {
long (*count_objects)(struct shrinker *, struct shrink_control *sc);
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 6bac41e..acb4aef 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -291,7 +291,7 @@ shrink_slab_one(struct shrinker *shrinker, struct
shrink_control *shrinkctl,
shrinkctl-nr_to_scan = nr_to_scan;
ret = shrinker-scan_objects(shrinker, shrinkctl);
- if (ret == -1)
+ if (ret 0)
break;
freed += ret;
--
1.8.1.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] mm: vmscan: handle any negative return value from scan_objects
On 13:52 Thu 16 May , Dave Chinner wrote: On Thu, May 16, 2013 at 10:42:16AM +0200, Oskar Andero wrote: The shrinkers must return -1 to indicate that it is busy. Instead, treat any negative value as busy. Why? The API defines return condition for aborting a scan and gives a specific value for doing that. i.e. explain why should change the API to over-specify the 'abort scan return value like this. As I pointed out earlier, looking in to the code (from master): if (shrink_ret == -1) break; if (shrink_ret nr_before) ret += nr_before - shrink_ret; This piece of code lacks a sanity check and will only function if shrink_ret is either greater than zero or exactly -1. If shrink_ret is e.g. -2 this will lead to undefined behaviour. FWIW, using any negative number for abort scan is a bad API design decision. It means that in future we can't introduce different negative return values in the API if we have a new to. i.e. each specific negative return value needs to have the potential for defining a different behaviour. An alternative to my patch would be to add: if (shrink_ret -1) /* handle illegal return code in some way */ So if any change needs to be made, it is to change the -1 return value to an enum and have the shrinkers return that enum when they want an abort. I am all for an enum, but I still believe we should handle the case where the shrinkers return something wicked. -Oskar -- 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: [RFC PATCH 0/2] return value from shrinkers
On 16:18 Wed 15 May , Glauber Costa wrote: > On 05/15/2013 06:10 PM, Oskar Andero wrote: > > On 17:03 Tue 14 May , Glauber Costa wrote: > >> On 05/13/2013 06:16 PM, Oskar Andero wrote: > >>> Hi, > >>> > >>> In a previous discussion on lkml it was noted that the shrinkers use the > >>> magic value "-1" to signal that something went wrong. > >>> > >>> This patch-set implements the suggestion of instead using errno.h values > >>> to return something more meaningful. > >>> > >>> The first patch simply changes the check from -1 to any negative value and > >>> updates the comment accordingly. > >>> > >>> The second patch updates the shrinkers to return an errno.h value instead > >>> of -1. Since this one spans over many different areas I need input on > >>> what is > >>> a meaningful return value. Right now I used -EBUSY on everything for > >>> consitency. > >>> > >>> What do you say? Is this a good idea or does it make no sense at all? > >>> > >>> Thanks! > >>> > >> > >> Right now me and Dave are completely reworking the way shrinkers > >> operate. I suggest, first of all, that you take a look at that cautiously. > > > > Sounds good. Where can one find the code for that? > > > linux-mm, linux-fsdevel > > Subject is "kmemcg shrinkers", but only the second part is memcg related. > > >> On the specifics of what you are doing here, what would be the benefit > >> of returning something other than -1 ? Is there anything we would do > >> differently for a return value lesser than 1? > > > > Firstly, what bugs me is the magic and unintuitiveness of using -1 rather > > than a > > more descriptive error code. IMO, even a #define SHRINK_ERROR -1 in some > > header > > file would be better. > > > > Expanding the test to <0 will open up for more granular error checks, > > like -EAGAIN, -EBUSY and so on. Currently, they would all be treated the > > same, > > but maybe in the future we would like to handle them differently? > > > > Then in the future we change it. > This is not a user visible API, we are free to change it at any time, > under any conditions. There is only value in supporting different error > codes if we intend to do something different about it. Otherwise, it is > just churn. > > Moreover, -1 does not necessarily mean error. It means "stop shrinking". > There are many non-error conditions in which it could happen. > Sure, maybe errno.h is not the right way to go. So, why not add the #define instead? E.g. STOP_SHRINKING or something better than -1. > > Finally, looking at the code: > > if (shrink_ret == -1) > > break; > > if (shrink_ret < nr_before) > > ret += nr_before - shrink_ret; > > > > This piece of code will only function if shrink_ret is either greater than > > zero > > or -1. If shrink_ret is -2 this will lead to undefined behaviour. > > > Except it never is. But since we are touching this code anyway, I see no > problems in expanding the test. What I don't see the point for, is the > other patch in your series in which you return error codes. > > >> So far, shrink_slab behaves the same, you are just expanding the test. > >> If you really want to push this through, I would suggest coming up with > >> a more concrete reason for why this is wanted. > > > > I don't know how well this patch is aligned with your current rework, but > > based on my comments above, I don't see a reason for not taking it. > > > I see no objections for PATCH #1 that expands the check, as a cautionary > measure. But I will oppose returning error codes from shrinkers without > a solid reason for doing so (meaning a use case in which we really > threat one of the errors differently) Sorry for being over-zealous about the return codes and I understand that it is really a minor thing and possibly also a philosophical question. My only "solid" reasons are unintuiveness and readability. That is how I came across it in the first place. If no-one backs me up on this I will drop the second patch and resend the first patch without RFC prefix. -Oskar -- 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: [RFC PATCH 0/2] return value from shrinkers
On 17:03 Tue 14 May , Glauber Costa wrote: > On 05/13/2013 06:16 PM, Oskar Andero wrote: > > Hi, > > > > In a previous discussion on lkml it was noted that the shrinkers use the > > magic value "-1" to signal that something went wrong. > > > > This patch-set implements the suggestion of instead using errno.h values > > to return something more meaningful. > > > > The first patch simply changes the check from -1 to any negative value and > > updates the comment accordingly. > > > > The second patch updates the shrinkers to return an errno.h value instead > > of -1. Since this one spans over many different areas I need input on what > > is > > a meaningful return value. Right now I used -EBUSY on everything for > > consitency. > > > > What do you say? Is this a good idea or does it make no sense at all? > > > > Thanks! > > > > Right now me and Dave are completely reworking the way shrinkers > operate. I suggest, first of all, that you take a look at that cautiously. Sounds good. Where can one find the code for that? > On the specifics of what you are doing here, what would be the benefit > of returning something other than -1 ? Is there anything we would do > differently for a return value lesser than 1? Firstly, what bugs me is the magic and unintuitiveness of using -1 rather than a more descriptive error code. IMO, even a #define SHRINK_ERROR -1 in some header file would be better. Expanding the test to <0 will open up for more granular error checks, like -EAGAIN, -EBUSY and so on. Currently, they would all be treated the same, but maybe in the future we would like to handle them differently? Finally, looking at the code: if (shrink_ret == -1) break; if (shrink_ret < nr_before) ret += nr_before - shrink_ret; This piece of code will only function if shrink_ret is either greater than zero or -1. If shrink_ret is -2 this will lead to undefined behaviour. > So far, shrink_slab behaves the same, you are just expanding the test. > If you really want to push this through, I would suggest coming up with > a more concrete reason for why this is wanted. I don't know how well this patch is aligned with your current rework, but based on my comments above, I don't see a reason for not taking it. -Oskar -- 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: [RFC PATCH 0/2] return value from shrinkers
On 17:03 Tue 14 May , Glauber Costa wrote: On 05/13/2013 06:16 PM, Oskar Andero wrote: Hi, In a previous discussion on lkml it was noted that the shrinkers use the magic value -1 to signal that something went wrong. This patch-set implements the suggestion of instead using errno.h values to return something more meaningful. The first patch simply changes the check from -1 to any negative value and updates the comment accordingly. The second patch updates the shrinkers to return an errno.h value instead of -1. Since this one spans over many different areas I need input on what is a meaningful return value. Right now I used -EBUSY on everything for consitency. What do you say? Is this a good idea or does it make no sense at all? Thanks! Right now me and Dave are completely reworking the way shrinkers operate. I suggest, first of all, that you take a look at that cautiously. Sounds good. Where can one find the code for that? On the specifics of what you are doing here, what would be the benefit of returning something other than -1 ? Is there anything we would do differently for a return value lesser than 1? Firstly, what bugs me is the magic and unintuitiveness of using -1 rather than a more descriptive error code. IMO, even a #define SHRINK_ERROR -1 in some header file would be better. Expanding the test to 0 will open up for more granular error checks, like -EAGAIN, -EBUSY and so on. Currently, they would all be treated the same, but maybe in the future we would like to handle them differently? Finally, looking at the code: if (shrink_ret == -1) break; if (shrink_ret nr_before) ret += nr_before - shrink_ret; This piece of code will only function if shrink_ret is either greater than zero or -1. If shrink_ret is -2 this will lead to undefined behaviour. So far, shrink_slab behaves the same, you are just expanding the test. If you really want to push this through, I would suggest coming up with a more concrete reason for why this is wanted. I don't know how well this patch is aligned with your current rework, but based on my comments above, I don't see a reason for not taking it. -Oskar -- 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: [RFC PATCH 0/2] return value from shrinkers
On 16:18 Wed 15 May , Glauber Costa wrote: On 05/15/2013 06:10 PM, Oskar Andero wrote: On 17:03 Tue 14 May , Glauber Costa wrote: On 05/13/2013 06:16 PM, Oskar Andero wrote: Hi, In a previous discussion on lkml it was noted that the shrinkers use the magic value -1 to signal that something went wrong. This patch-set implements the suggestion of instead using errno.h values to return something more meaningful. The first patch simply changes the check from -1 to any negative value and updates the comment accordingly. The second patch updates the shrinkers to return an errno.h value instead of -1. Since this one spans over many different areas I need input on what is a meaningful return value. Right now I used -EBUSY on everything for consitency. What do you say? Is this a good idea or does it make no sense at all? Thanks! Right now me and Dave are completely reworking the way shrinkers operate. I suggest, first of all, that you take a look at that cautiously. Sounds good. Where can one find the code for that? linux-mm, linux-fsdevel Subject is kmemcg shrinkers, but only the second part is memcg related. On the specifics of what you are doing here, what would be the benefit of returning something other than -1 ? Is there anything we would do differently for a return value lesser than 1? Firstly, what bugs me is the magic and unintuitiveness of using -1 rather than a more descriptive error code. IMO, even a #define SHRINK_ERROR -1 in some header file would be better. Expanding the test to 0 will open up for more granular error checks, like -EAGAIN, -EBUSY and so on. Currently, they would all be treated the same, but maybe in the future we would like to handle them differently? Then in the future we change it. This is not a user visible API, we are free to change it at any time, under any conditions. There is only value in supporting different error codes if we intend to do something different about it. Otherwise, it is just churn. Moreover, -1 does not necessarily mean error. It means stop shrinking. There are many non-error conditions in which it could happen. Sure, maybe errno.h is not the right way to go. So, why not add the #define instead? E.g. STOP_SHRINKING or something better than -1. Finally, looking at the code: if (shrink_ret == -1) break; if (shrink_ret nr_before) ret += nr_before - shrink_ret; This piece of code will only function if shrink_ret is either greater than zero or -1. If shrink_ret is -2 this will lead to undefined behaviour. Except it never is. But since we are touching this code anyway, I see no problems in expanding the test. What I don't see the point for, is the other patch in your series in which you return error codes. So far, shrink_slab behaves the same, you are just expanding the test. If you really want to push this through, I would suggest coming up with a more concrete reason for why this is wanted. I don't know how well this patch is aligned with your current rework, but based on my comments above, I don't see a reason for not taking it. I see no objections for PATCH #1 that expands the check, as a cautionary measure. But I will oppose returning error codes from shrinkers without a solid reason for doing so (meaning a use case in which we really threat one of the errors differently) Sorry for being over-zealous about the return codes and I understand that it is really a minor thing and possibly also a philosophical question. My only solid reasons are unintuiveness and readability. That is how I came across it in the first place. If no-one backs me up on this I will drop the second patch and resend the first patch without RFC prefix. -Oskar -- 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/
[RFC PATCH 0/2] return value from shrinkers
Hi, In a previous discussion on lkml it was noted that the shrinkers use the magic value "-1" to signal that something went wrong. This patch-set implements the suggestion of instead using errno.h values to return something more meaningful. The first patch simply changes the check from -1 to any negative value and updates the comment accordingly. The second patch updates the shrinkers to return an errno.h value instead of -1. Since this one spans over many different areas I need input on what is a meaningful return value. Right now I used -EBUSY on everything for consitency. What do you say? Is this a good idea or does it make no sense at all? Thanks! -Oskar Oskar Andero (2): mm: vmscan: let any negative return value from shrinker mean error Clean-up shrinker return values drivers/staging/android/ashmem.c | 2 +- drivers/staging/zcache/zcache-main.c | 2 +- fs/gfs2/glock.c | 2 +- fs/gfs2/quota.c | 2 +- fs/nfs/dir.c | 2 +- fs/ubifs/shrinker.c | 2 +- include/linux/shrinker.h | 5 +++-- mm/vmscan.c | 2 +- net/sunrpc/auth.c| 2 +- 9 files changed, 11 insertions(+), 10 deletions(-) -- 1.8.1.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/
[RFC PATCH 1/2] mm: vmscan: let any negative return value from shrinker mean error
The shrinkers must return -1 to indicate that it is busy. Instead of
relaying on magical numbers, let any negative value indicate error. This
opens up for using the errno.h error codes in the shrinker
implementations.
Cc: Hugh Dickins
Cc: Greg Kroah-Hartman
Signed-off-by: Oskar Andero
---
include/linux/shrinker.h | 5 +++--
mm/vmscan.c | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
index ac6b8ee..31e9406 100644
--- a/include/linux/shrinker.h
+++ b/include/linux/shrinker.h
@@ -18,8 +18,9 @@ struct shrink_control {
* 'sc' is passed shrink_control which includes a count 'nr_to_scan'
* and a 'gfpmask'. It should look through the least-recently-used
* 'nr_to_scan' entries and attempt to free them up. It should return
- * the number of objects which remain in the cache. If it returns -1, it means
- * it cannot do any scanning at this time (eg. there is a risk of deadlock).
+ * the number of objects which remain in the cache. If it returns a
+ * negative error code, it means it cannot do any scanning at this time
+ * (eg. there is a risk of deadlock).
*
* The 'gfpmask' refers to the allocation we are currently trying to
* fulfil.
diff --git a/mm/vmscan.c b/mm/vmscan.c
index fa6a853..d6ac9a8 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -287,7 +287,7 @@ unsigned long shrink_slab(struct shrink_control *shrink,
nr_before = do_shrinker_shrink(shrinker, shrink, 0);
shrink_ret = do_shrinker_shrink(shrinker, shrink,
batch_size);
- if (shrink_ret == -1)
+ if (shrink_ret < 0)
break;
if (shrink_ret < nr_before)
ret += nr_before - shrink_ret;
--
1.8.1.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/
[RFC PATCH 2/2] Clean-up shrinker return values
Shrinkers return hardcoded -1 on error. Use errno.h values instead
to add more meaning to the errors.
Cc: Hugh Dickins
Cc: Greg Kroah-Hartman
Signed-off-by: Oskar Andero
---
drivers/staging/android/ashmem.c | 2 +-
drivers/staging/zcache/zcache-main.c | 2 +-
fs/gfs2/glock.c | 2 +-
fs/gfs2/quota.c | 2 +-
fs/nfs/dir.c | 2 +-
fs/ubifs/shrinker.c | 2 +-
net/sunrpc/auth.c| 2 +-
7 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index e681bdd..1968d2f 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -359,7 +359,7 @@ static int ashmem_shrink(struct shrinker *s, struct
shrink_control *sc)
/* We might recurse into filesystem code, so bail out if necessary */
if (sc->nr_to_scan && !(sc->gfp_mask & __GFP_FS))
- return -1;
+ return -EBUSY;
if (!sc->nr_to_scan)
return lru_count;
diff --git a/drivers/staging/zcache/zcache-main.c
b/drivers/staging/zcache/zcache-main.c
index 522cb8e..a38532c 100644
--- a/drivers/staging/zcache/zcache-main.c
+++ b/drivers/staging/zcache/zcache-main.c
@@ -1144,7 +1144,7 @@ static int shrink_zcache_memory(struct shrinker *shrink,
struct shrink_control *sc)
{
static bool in_progress;
- int ret = -1;
+ int ret = -EBUSY;
int nr = sc->nr_to_scan;
int nr_evict = 0;
int nr_writeback = 0;
diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 9435384..401b089 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -1459,7 +1459,7 @@ static int gfs2_shrink_glock_memory(struct shrinker
*shrink,
{
if (sc->nr_to_scan) {
if (!(sc->gfp_mask & __GFP_FS))
- return -1;
+ return -EBUSY;
gfs2_scan_glock_lru(sc->nr_to_scan);
}
diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
index c7c840e..14acbb2 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -85,7 +85,7 @@ int gfs2_shrink_qd_memory(struct shrinker *shrink, struct
shrink_control *sc)
goto out;
if (!(sc->gfp_mask & __GFP_FS))
- return -1;
+ return -EBUSY;
spin_lock(_lru_lock);
while (nr_to_scan && !list_empty(_lru_list)) {
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index e093e73..9fee9bc 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1968,7 +1968,7 @@ int nfs_access_cache_shrinker(struct shrinker *shrink,
gfp_t gfp_mask = sc->gfp_mask;
if ((gfp_mask & GFP_KERNEL) != GFP_KERNEL)
- return (nr_to_scan == 0) ? 0 : -1;
+ return (nr_to_scan == 0) ? nr_to_scan : -EBUSY;
spin_lock(_access_lru_lock);
list_for_each_entry_safe(nfsi, next, _access_lru_list,
access_cache_inode_lru) {
diff --git a/fs/ubifs/shrinker.c b/fs/ubifs/shrinker.c
index 9e1d056..294e685 100644
--- a/fs/ubifs/shrinker.c
+++ b/fs/ubifs/shrinker.c
@@ -316,7 +316,7 @@ int ubifs_shrinker(struct shrinker *shrink, struct
shrink_control *sc)
if (!freed && contention) {
dbg_tnc("freed nothing, but contention");
- return -1;
+ return -EBUSY;
}
out:
diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
index ed2fdd2..45faea0 100644
--- a/net/sunrpc/auth.c
+++ b/net/sunrpc/auth.c
@@ -461,7 +461,7 @@ rpcauth_cache_shrinker(struct shrinker *shrink, struct
shrink_control *sc)
gfp_t gfp_mask = sc->gfp_mask;
if ((gfp_mask & GFP_KERNEL) != GFP_KERNEL)
- return (nr_to_scan == 0) ? 0 : -1;
+ return (nr_to_scan == 0) ? nr_to_scan : -EBUSY;
if (list_empty(_unused))
return 0;
spin_lock(_credcache_lock);
--
1.8.1.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/
[RFC PATCH 2/2] Clean-up shrinker return values
Shrinkers return hardcoded -1 on error. Use errno.h values instead
to add more meaning to the errors.
Cc: Hugh Dickins hu...@google.com
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
---
drivers/staging/android/ashmem.c | 2 +-
drivers/staging/zcache/zcache-main.c | 2 +-
fs/gfs2/glock.c | 2 +-
fs/gfs2/quota.c | 2 +-
fs/nfs/dir.c | 2 +-
fs/ubifs/shrinker.c | 2 +-
net/sunrpc/auth.c| 2 +-
7 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index e681bdd..1968d2f 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -359,7 +359,7 @@ static int ashmem_shrink(struct shrinker *s, struct
shrink_control *sc)
/* We might recurse into filesystem code, so bail out if necessary */
if (sc-nr_to_scan !(sc-gfp_mask __GFP_FS))
- return -1;
+ return -EBUSY;
if (!sc-nr_to_scan)
return lru_count;
diff --git a/drivers/staging/zcache/zcache-main.c
b/drivers/staging/zcache/zcache-main.c
index 522cb8e..a38532c 100644
--- a/drivers/staging/zcache/zcache-main.c
+++ b/drivers/staging/zcache/zcache-main.c
@@ -1144,7 +1144,7 @@ static int shrink_zcache_memory(struct shrinker *shrink,
struct shrink_control *sc)
{
static bool in_progress;
- int ret = -1;
+ int ret = -EBUSY;
int nr = sc-nr_to_scan;
int nr_evict = 0;
int nr_writeback = 0;
diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 9435384..401b089 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -1459,7 +1459,7 @@ static int gfs2_shrink_glock_memory(struct shrinker
*shrink,
{
if (sc-nr_to_scan) {
if (!(sc-gfp_mask __GFP_FS))
- return -1;
+ return -EBUSY;
gfs2_scan_glock_lru(sc-nr_to_scan);
}
diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
index c7c840e..14acbb2 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -85,7 +85,7 @@ int gfs2_shrink_qd_memory(struct shrinker *shrink, struct
shrink_control *sc)
goto out;
if (!(sc-gfp_mask __GFP_FS))
- return -1;
+ return -EBUSY;
spin_lock(qd_lru_lock);
while (nr_to_scan !list_empty(qd_lru_list)) {
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index e093e73..9fee9bc 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1968,7 +1968,7 @@ int nfs_access_cache_shrinker(struct shrinker *shrink,
gfp_t gfp_mask = sc-gfp_mask;
if ((gfp_mask GFP_KERNEL) != GFP_KERNEL)
- return (nr_to_scan == 0) ? 0 : -1;
+ return (nr_to_scan == 0) ? nr_to_scan : -EBUSY;
spin_lock(nfs_access_lru_lock);
list_for_each_entry_safe(nfsi, next, nfs_access_lru_list,
access_cache_inode_lru) {
diff --git a/fs/ubifs/shrinker.c b/fs/ubifs/shrinker.c
index 9e1d056..294e685 100644
--- a/fs/ubifs/shrinker.c
+++ b/fs/ubifs/shrinker.c
@@ -316,7 +316,7 @@ int ubifs_shrinker(struct shrinker *shrink, struct
shrink_control *sc)
if (!freed contention) {
dbg_tnc(freed nothing, but contention);
- return -1;
+ return -EBUSY;
}
out:
diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
index ed2fdd2..45faea0 100644
--- a/net/sunrpc/auth.c
+++ b/net/sunrpc/auth.c
@@ -461,7 +461,7 @@ rpcauth_cache_shrinker(struct shrinker *shrink, struct
shrink_control *sc)
gfp_t gfp_mask = sc-gfp_mask;
if ((gfp_mask GFP_KERNEL) != GFP_KERNEL)
- return (nr_to_scan == 0) ? 0 : -1;
+ return (nr_to_scan == 0) ? nr_to_scan : -EBUSY;
if (list_empty(cred_unused))
return 0;
spin_lock(rpc_credcache_lock);
--
1.8.1.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/
[RFC PATCH 0/2] return value from shrinkers
Hi, In a previous discussion on lkml it was noted that the shrinkers use the magic value -1 to signal that something went wrong. This patch-set implements the suggestion of instead using errno.h values to return something more meaningful. The first patch simply changes the check from -1 to any negative value and updates the comment accordingly. The second patch updates the shrinkers to return an errno.h value instead of -1. Since this one spans over many different areas I need input on what is a meaningful return value. Right now I used -EBUSY on everything for consitency. What do you say? Is this a good idea or does it make no sense at all? Thanks! -Oskar Oskar Andero (2): mm: vmscan: let any negative return value from shrinker mean error Clean-up shrinker return values drivers/staging/android/ashmem.c | 2 +- drivers/staging/zcache/zcache-main.c | 2 +- fs/gfs2/glock.c | 2 +- fs/gfs2/quota.c | 2 +- fs/nfs/dir.c | 2 +- fs/ubifs/shrinker.c | 2 +- include/linux/shrinker.h | 5 +++-- mm/vmscan.c | 2 +- net/sunrpc/auth.c| 2 +- 9 files changed, 11 insertions(+), 10 deletions(-) -- 1.8.1.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/
[RFC PATCH 1/2] mm: vmscan: let any negative return value from shrinker mean error
The shrinkers must return -1 to indicate that it is busy. Instead of
relaying on magical numbers, let any negative value indicate error. This
opens up for using the errno.h error codes in the shrinker
implementations.
Cc: Hugh Dickins hu...@google.com
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
---
include/linux/shrinker.h | 5 +++--
mm/vmscan.c | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
index ac6b8ee..31e9406 100644
--- a/include/linux/shrinker.h
+++ b/include/linux/shrinker.h
@@ -18,8 +18,9 @@ struct shrink_control {
* 'sc' is passed shrink_control which includes a count 'nr_to_scan'
* and a 'gfpmask'. It should look through the least-recently-used
* 'nr_to_scan' entries and attempt to free them up. It should return
- * the number of objects which remain in the cache. If it returns -1, it means
- * it cannot do any scanning at this time (eg. there is a risk of deadlock).
+ * the number of objects which remain in the cache. If it returns a
+ * negative error code, it means it cannot do any scanning at this time
+ * (eg. there is a risk of deadlock).
*
* The 'gfpmask' refers to the allocation we are currently trying to
* fulfil.
diff --git a/mm/vmscan.c b/mm/vmscan.c
index fa6a853..d6ac9a8 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -287,7 +287,7 @@ unsigned long shrink_slab(struct shrink_control *shrink,
nr_before = do_shrinker_shrink(shrinker, shrink, 0);
shrink_ret = do_shrinker_shrink(shrinker, shrink,
batch_size);
- if (shrink_ret == -1)
+ if (shrink_ret 0)
break;
if (shrink_ret nr_before)
ret += nr_before - shrink_ret;
--
1.8.1.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/
[PATCHv4] iio: adc: add driver for MCP3204/08 12-bit ADC
This adds support for Microchip's 12 bit AD converters MCP3204 and
MCP3208. These chips communicates over SPI and supports single-ended
and pseudo-differential configurations.
Cc: Jonathan Cameron
Cc: Lars-Peter Clausen
Signed-off-by: Oskar Andero
---
drivers/iio/adc/Kconfig | 10 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/mcp320x.c | 257 ++
3 files changed, 268 insertions(+)
create mode 100644 drivers/iio/adc/mcp320x.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ab0767e6..93129ec 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -133,6 +133,16 @@ config MAX1363
max11646, max11647) Provides direct access via sysfs and buffered
data via the iio dev interface.
+config MCP320X
+ tristate "Microchip Technology MCP3204/08"
+ depends on SPI
+ help
+ Say yes here to build support for Microchip Technology's MCP3204 or
+ MCP3208 analog to digital converter.
+
+ This driver can also be built as a module. If so, the module will be
+ called mcp320x.
+
config TI_ADC081C
tristate "Texas Instruments ADC081C021/027"
depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 0a825be..8f475d3 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_AT91_ADC) += at91_adc.o
obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
obj-$(CONFIG_MAX1363) += max1363.o
+obj-$(CONFIG_MCP320X) += mcp320x.o
obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
new file mode 100644
index 000..ebc0159
--- /dev/null
+++ b/drivers/iio/adc/mcp320x.c
@@ -0,0 +1,257 @@
+/*
+ * Copyright (C) 2013 Oskar Andero
+ *
+ * Driver for Microchip Technology's MCP3204 and MCP3208 ADC chips.
+ * Datasheet can be found here:
+ * http://ww1.microchip.com/downloads/en/devicedoc/21298c.pdf
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#define MCP_SINGLE_ENDED (1 << 3)
+#define MCP_START_BIT (1 << 4)
+
+enum {
+ mcp3204,
+ mcp3208,
+};
+
+struct mcp320x {
+ struct spi_device *spi;
+ struct spi_message msg;
+ struct spi_transfer transfer[2];
+
+ u8 tx_buf;
+ u8 rx_buf[2];
+
+ struct regulator *reg;
+ struct mutex lock;
+};
+
+static int mcp320x_adc_conversion(struct mcp320x *adc, u8 msg)
+{
+ int ret;
+
+ adc->tx_buf = msg;
+ ret = spi_sync(adc->spi, >msg);
+ if (ret < 0)
+ return ret;
+
+ return ((adc->rx_buf[0] & 0x3f) << 6) |
+ (adc->rx_buf[1] >> 2);
+}
+
+static int mcp320x_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *channel, int *val,
+ int *val2, long mask)
+{
+ struct mcp320x *adc = iio_priv(indio_dev);
+ int ret = -EINVAL;
+
+ mutex_lock(>lock);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ if (channel->differential)
+ ret = mcp320x_adc_conversion(adc,
+ MCP_START_BIT | channel->address);
+ else
+ ret = mcp320x_adc_conversion(adc,
+ MCP_START_BIT | MCP_SINGLE_ENDED |
+ channel->address);
+ if (ret < 0)
+ goto out;
+
+ *val = ret;
+ ret = IIO_VAL_INT;
+ break;
+
+ case IIO_CHAN_INFO_SCALE:
+ /* Digital output code = (4096 * Vin) / Vref */
+ ret = regulator_get_voltage(adc->reg);
+ if (ret < 0)
+ goto out;
+
+ *val = ret / 1000;
+ *val2 = 12;
+ ret = IIO_VAL_FRACTIONAL_LOG2;
+ break;
+
+ default:
+ break;
+ }
+
+out:
+ mutex_unlock(>lock);
+
+ return ret;
+}
+
+#define MCP320X_VOLTAGE_CHANNEL(num) \
+ { \
+ .type = IIO_VOLTAGE,\
+ .indexed = 1, \
+ .channel = (num), \
+ .address = (num), \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_
[PATCHv3] iio: adc: add driver for MCP3204/08 12-bit ADC
This adds support for Microchip's 12 bit AD converters MCP3204 and
MCP3208. These chips communicates over SPI and supports single-ended
and pseudo-differential configurations.
Cc: Jonathan Cameron
Cc: Lars-Peter Clausen
Signed-off-by: Oskar Andero
---
drivers/iio/adc/Kconfig | 10 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/mcp320x.c | 258 ++
3 files changed, 269 insertions(+)
create mode 100644 drivers/iio/adc/mcp320x.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ab0767e6..93129ec 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -133,6 +133,16 @@ config MAX1363
max11646, max11647) Provides direct access via sysfs and buffered
data via the iio dev interface.
+config MCP320X
+ tristate "Microchip Technology MCP3204/08"
+ depends on SPI
+ help
+ Say yes here to build support for Microchip Technology's MCP3204 or
+ MCP3208 analog to digital converter.
+
+ This driver can also be built as a module. If so, the module will be
+ called mcp320x.
+
config TI_ADC081C
tristate "Texas Instruments ADC081C021/027"
depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 0a825be..8f475d3 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_AT91_ADC) += at91_adc.o
obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
obj-$(CONFIG_MAX1363) += max1363.o
+obj-$(CONFIG_MCP320X) += mcp320x.o
obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
new file mode 100644
index 000..b38bf87
--- /dev/null
+++ b/drivers/iio/adc/mcp320x.c
@@ -0,0 +1,258 @@
+/*
+ * Copyright (C) 2013 Oskar Andero
+ *
+ * Driver for Microchip Technology's MCP3204 and MCP3208 ADC chips.
+ * Datasheet can be found here:
+ * http://ww1.microchip.com/downloads/en/devicedoc/21298c.pdf
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#define MCP_SINGLE_ENDED (1 << 3)
+#define MCP_START_BIT (1 << 4)
+
+enum {
+ mcp3204,
+ mcp3208,
+};
+
+struct mcp320x {
+ struct spi_device *spi;
+ struct spi_message msg;
+ struct spi_transfer transfer[2];
+
+ u8 tx_buf;
+ u8 rx_buf[2];
+
+ struct regulator *reg;
+ unsigned int ref_mv;
+
+ struct mutex lock;
+};
+
+static int mcp320x_adc_conversion(struct mcp320x *adc, u8 msg)
+{
+ int ret;
+
+ adc->tx_buf = msg;
+ ret = spi_sync(adc->spi, >msg);
+ if (ret < 0)
+ return ret;
+
+ return ((adc->rx_buf[0] & 0x3f) << 6) |
+ (adc->rx_buf[1] >> 2);
+}
+
+static int mcp320x_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *channel, int *val,
+ int *val2, long mask)
+{
+ struct mcp320x *adc = iio_priv(indio_dev);
+ int ret = -EINVAL;
+
+ mutex_lock(>lock);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ if (channel->differential)
+ ret = mcp320x_adc_conversion(adc,
+ MCP_START_BIT | channel->address);
+ else
+ ret = mcp320x_adc_conversion(adc,
+ MCP_START_BIT | MCP_SINGLE_ENDED |
+ channel->address);
+ if (ret < 0)
+ goto out;
+
+ *val = ret;
+ ret = IIO_VAL_INT;
+ break;
+
+ case IIO_CHAN_INFO_SCALE:
+ /* Digital output code = (4096 * Vin) / Vref */
+ ret = regulator_get_voltage(adc->reg);
+ if (ret < 0)
+ goto out;
+
+ *val = ret / 1000;
+ *val2 = 12;
+ ret = IIO_VAL_FRACTIONAL_LOG2;
+ break;
+
+ default:
+ break;
+ }
+
+out:
+ mutex_unlock(>lock);
+
+ return ret;
+}
+
+#define MCP320X_VOLTAGE_CHANNEL(num) \
+ { \
+ .type = IIO_VOLTAGE,\
+ .indexed = 1, \
+ .channel = (num), \
+ .address = (num), \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_
[PATCHv3] iio: adc: add driver for MCP3204/08 12-bit ADC
This adds support for Microchip's 12 bit AD converters MCP3204 and
MCP3208. These chips communicates over SPI and supports single-ended
and pseudo-differential configurations.
Cc: Jonathan Cameron ji...@cam.ac.uk
Cc: Lars-Peter Clausen l...@metafoo.de
Signed-off-by: Oskar Andero oskar.and...@gmail.com
---
drivers/iio/adc/Kconfig | 10 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/mcp320x.c | 258 ++
3 files changed, 269 insertions(+)
create mode 100644 drivers/iio/adc/mcp320x.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ab0767e6..93129ec 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -133,6 +133,16 @@ config MAX1363
max11646, max11647) Provides direct access via sysfs and buffered
data via the iio dev interface.
+config MCP320X
+ tristate Microchip Technology MCP3204/08
+ depends on SPI
+ help
+ Say yes here to build support for Microchip Technology's MCP3204 or
+ MCP3208 analog to digital converter.
+
+ This driver can also be built as a module. If so, the module will be
+ called mcp320x.
+
config TI_ADC081C
tristate Texas Instruments ADC081C021/027
depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 0a825be..8f475d3 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_AT91_ADC) += at91_adc.o
obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
obj-$(CONFIG_MAX1363) += max1363.o
+obj-$(CONFIG_MCP320X) += mcp320x.o
obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
new file mode 100644
index 000..b38bf87
--- /dev/null
+++ b/drivers/iio/adc/mcp320x.c
@@ -0,0 +1,258 @@
+/*
+ * Copyright (C) 2013 Oskar Andero oskar.and...@gmail.com
+ *
+ * Driver for Microchip Technology's MCP3204 and MCP3208 ADC chips.
+ * Datasheet can be found here:
+ * http://ww1.microchip.com/downloads/en/devicedoc/21298c.pdf
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/err.h
+#include linux/spi/spi.h
+#include linux/module.h
+#include linux/iio/iio.h
+#include linux/regulator/consumer.h
+
+#define MCP_SINGLE_ENDED (1 3)
+#define MCP_START_BIT (1 4)
+
+enum {
+ mcp3204,
+ mcp3208,
+};
+
+struct mcp320x {
+ struct spi_device *spi;
+ struct spi_message msg;
+ struct spi_transfer transfer[2];
+
+ u8 tx_buf;
+ u8 rx_buf[2];
+
+ struct regulator *reg;
+ unsigned int ref_mv;
+
+ struct mutex lock;
+};
+
+static int mcp320x_adc_conversion(struct mcp320x *adc, u8 msg)
+{
+ int ret;
+
+ adc-tx_buf = msg;
+ ret = spi_sync(adc-spi, adc-msg);
+ if (ret 0)
+ return ret;
+
+ return ((adc-rx_buf[0] 0x3f) 6) |
+ (adc-rx_buf[1] 2);
+}
+
+static int mcp320x_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *channel, int *val,
+ int *val2, long mask)
+{
+ struct mcp320x *adc = iio_priv(indio_dev);
+ int ret = -EINVAL;
+
+ mutex_lock(adc-lock);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ if (channel-differential)
+ ret = mcp320x_adc_conversion(adc,
+ MCP_START_BIT | channel-address);
+ else
+ ret = mcp320x_adc_conversion(adc,
+ MCP_START_BIT | MCP_SINGLE_ENDED |
+ channel-address);
+ if (ret 0)
+ goto out;
+
+ *val = ret;
+ ret = IIO_VAL_INT;
+ break;
+
+ case IIO_CHAN_INFO_SCALE:
+ /* Digital output code = (4096 * Vin) / Vref */
+ ret = regulator_get_voltage(adc-reg);
+ if (ret 0)
+ goto out;
+
+ *val = ret / 1000;
+ *val2 = 12;
+ ret = IIO_VAL_FRACTIONAL_LOG2;
+ break;
+
+ default:
+ break;
+ }
+
+out:
+ mutex_unlock(adc-lock);
+
+ return ret;
+}
+
+#define MCP320X_VOLTAGE_CHANNEL(num) \
+ { \
+ .type = IIO_VOLTAGE,\
+ .indexed = 1, \
+ .channel = (num), \
+ .address = (num), \
+ .info_mask_separate = BIT
[PATCHv4] iio: adc: add driver for MCP3204/08 12-bit ADC
This adds support for Microchip's 12 bit AD converters MCP3204 and
MCP3208. These chips communicates over SPI and supports single-ended
and pseudo-differential configurations.
Cc: Jonathan Cameron ji...@cam.ac.uk
Cc: Lars-Peter Clausen l...@metafoo.de
Signed-off-by: Oskar Andero oskar.and...@gmail.com
---
drivers/iio/adc/Kconfig | 10 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/mcp320x.c | 257 ++
3 files changed, 268 insertions(+)
create mode 100644 drivers/iio/adc/mcp320x.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ab0767e6..93129ec 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -133,6 +133,16 @@ config MAX1363
max11646, max11647) Provides direct access via sysfs and buffered
data via the iio dev interface.
+config MCP320X
+ tristate Microchip Technology MCP3204/08
+ depends on SPI
+ help
+ Say yes here to build support for Microchip Technology's MCP3204 or
+ MCP3208 analog to digital converter.
+
+ This driver can also be built as a module. If so, the module will be
+ called mcp320x.
+
config TI_ADC081C
tristate Texas Instruments ADC081C021/027
depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 0a825be..8f475d3 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_AT91_ADC) += at91_adc.o
obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
obj-$(CONFIG_MAX1363) += max1363.o
+obj-$(CONFIG_MCP320X) += mcp320x.o
obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
new file mode 100644
index 000..ebc0159
--- /dev/null
+++ b/drivers/iio/adc/mcp320x.c
@@ -0,0 +1,257 @@
+/*
+ * Copyright (C) 2013 Oskar Andero oskar.and...@gmail.com
+ *
+ * Driver for Microchip Technology's MCP3204 and MCP3208 ADC chips.
+ * Datasheet can be found here:
+ * http://ww1.microchip.com/downloads/en/devicedoc/21298c.pdf
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/err.h
+#include linux/spi/spi.h
+#include linux/module.h
+#include linux/iio/iio.h
+#include linux/regulator/consumer.h
+
+#define MCP_SINGLE_ENDED (1 3)
+#define MCP_START_BIT (1 4)
+
+enum {
+ mcp3204,
+ mcp3208,
+};
+
+struct mcp320x {
+ struct spi_device *spi;
+ struct spi_message msg;
+ struct spi_transfer transfer[2];
+
+ u8 tx_buf;
+ u8 rx_buf[2];
+
+ struct regulator *reg;
+ struct mutex lock;
+};
+
+static int mcp320x_adc_conversion(struct mcp320x *adc, u8 msg)
+{
+ int ret;
+
+ adc-tx_buf = msg;
+ ret = spi_sync(adc-spi, adc-msg);
+ if (ret 0)
+ return ret;
+
+ return ((adc-rx_buf[0] 0x3f) 6) |
+ (adc-rx_buf[1] 2);
+}
+
+static int mcp320x_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *channel, int *val,
+ int *val2, long mask)
+{
+ struct mcp320x *adc = iio_priv(indio_dev);
+ int ret = -EINVAL;
+
+ mutex_lock(adc-lock);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ if (channel-differential)
+ ret = mcp320x_adc_conversion(adc,
+ MCP_START_BIT | channel-address);
+ else
+ ret = mcp320x_adc_conversion(adc,
+ MCP_START_BIT | MCP_SINGLE_ENDED |
+ channel-address);
+ if (ret 0)
+ goto out;
+
+ *val = ret;
+ ret = IIO_VAL_INT;
+ break;
+
+ case IIO_CHAN_INFO_SCALE:
+ /* Digital output code = (4096 * Vin) / Vref */
+ ret = regulator_get_voltage(adc-reg);
+ if (ret 0)
+ goto out;
+
+ *val = ret / 1000;
+ *val2 = 12;
+ ret = IIO_VAL_FRACTIONAL_LOG2;
+ break;
+
+ default:
+ break;
+ }
+
+out:
+ mutex_unlock(adc-lock);
+
+ return ret;
+}
+
+#define MCP320X_VOLTAGE_CHANNEL(num) \
+ { \
+ .type = IIO_VOLTAGE,\
+ .indexed = 1, \
+ .channel = (num), \
+ .address = (num), \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW
[PATCHv2] iio: adc: add driver for MCP3204/08 12-bit ADC
This adds support for Microchip's 12 bit AD converters MCP3204 and
MCP3208. These chips communicates over SPI and supports single-ended
and pseudo-differential configurations.
Cc: Jonathan Cameron
Cc: Lars-Peter Clausen
Signed-off-by: Oskar Andero
---
drivers/iio/adc/Kconfig | 10 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/mcp320x.c | 255 ++
3 files changed, 266 insertions(+)
create mode 100644 drivers/iio/adc/mcp320x.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ab0767e6..93129ec 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -133,6 +133,16 @@ config MAX1363
max11646, max11647) Provides direct access via sysfs and buffered
data via the iio dev interface.
+config MCP320X
+ tristate "Microchip Technology MCP3204/08"
+ depends on SPI
+ help
+ Say yes here to build support for Microchip Technology's MCP3204 or
+ MCP3208 analog to digital converter.
+
+ This driver can also be built as a module. If so, the module will be
+ called mcp320x.
+
config TI_ADC081C
tristate "Texas Instruments ADC081C021/027"
depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 0a825be..8f475d3 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_AT91_ADC) += at91_adc.o
obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
obj-$(CONFIG_MAX1363) += max1363.o
+obj-$(CONFIG_MCP320X) += mcp320x.o
obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
new file mode 100644
index 000..7dc32c3
--- /dev/null
+++ b/drivers/iio/adc/mcp320x.c
@@ -0,0 +1,255 @@
+/*
+ * Copyright (C) 2013 Oskar Andero
+ *
+ * Driver for Microchip Technology's MCP3204 and MCP3208 ADC chips.
+ * Datasheet can be found here:
+ * http://ww1.microchip.com/downloads/en/devicedoc/21298c.pdf
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#define MCP_SINGLE_ENDED (1 << 3)
+#define MCP_START_BIT (1 << 4)
+
+enum {
+ mcp3204,
+ mcp3208,
+};
+
+struct mcp320x {
+ struct spi_device *spi;
+ struct spi_message msg;
+ struct spi_transfer transfer[2];
+
+ u8 tx_buf;
+ u8 rx_buf[2];
+
+ struct regulator *reg;
+ unsigned int ref_mv;
+
+ struct mutex lock;
+};
+
+static int mcp320x_adc_conversion(struct mcp320x *adc, u8 msg)
+{
+ int ret;
+
+ adc->tx_buf = msg;
+ ret = spi_sync(adc->spi, >msg);
+ if (ret < 0)
+ return ret;
+
+ return ((adc->rx_buf[0] & 0x3f) << 6) |
+ (adc->rx_buf[1] >> 2);
+}
+
+static int mcp320x_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *channel, int *val,
+ int *val2, long mask)
+{
+ struct mcp320x *adc = iio_priv(indio_dev);
+ int ret = -EINVAL;
+
+ mutex_lock(>lock);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ if (channel->differential)
+ ret = mcp320x_adc_conversion(adc,
+ MCP_START_BIT | channel->address);
+ else
+ ret = mcp320x_adc_conversion(adc,
+ MCP_START_BIT | MCP_SINGLE_ENDED |
+ channel->address);
+ if (ret < 0)
+ goto out;
+
+ *val = ret;
+ ret = IIO_VAL_INT;
+ break;
+
+ case IIO_CHAN_INFO_SCALE:
+ /* Digital output code = (4096 * Vin) / Vref */
+ ret = regulator_get_voltage(adc->reg);
+ if (ret < 0)
+ goto out;
+
+ *val = ret / 1000;
+ *val2 = 4096;
+ ret = IIO_VAL_FRACTIONAL;
+ break;
+
+ default:
+ break;
+ }
+
+out:
+ mutex_unlock(>lock);
+
+ return ret;
+}
+
+#define MCP320X_VOLTAGE_CHANNEL(num) \
+ { \
+ .type = IIO_VOLTAGE,\
+ .indexed = 1, \
+ .channel = (num), \
+ .address = (num), \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_
[PATCHv2] iio: adc: add driver for MCP3204/08 12-bit ADC
This adds support for Microchip's 12 bit AD converters MCP3204 and
MCP3208. These chips communicates over SPI and supports single-ended
and pseudo-differential configurations.
Cc: Jonathan Cameron ji...@cam.ac.uk
Cc: Lars-Peter Clausen l...@metafoo.de
Signed-off-by: Oskar Andero oskar.and...@gmail.com
---
drivers/iio/adc/Kconfig | 10 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/mcp320x.c | 255 ++
3 files changed, 266 insertions(+)
create mode 100644 drivers/iio/adc/mcp320x.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ab0767e6..93129ec 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -133,6 +133,16 @@ config MAX1363
max11646, max11647) Provides direct access via sysfs and buffered
data via the iio dev interface.
+config MCP320X
+ tristate Microchip Technology MCP3204/08
+ depends on SPI
+ help
+ Say yes here to build support for Microchip Technology's MCP3204 or
+ MCP3208 analog to digital converter.
+
+ This driver can also be built as a module. If so, the module will be
+ called mcp320x.
+
config TI_ADC081C
tristate Texas Instruments ADC081C021/027
depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 0a825be..8f475d3 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_AT91_ADC) += at91_adc.o
obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
obj-$(CONFIG_MAX1363) += max1363.o
+obj-$(CONFIG_MCP320X) += mcp320x.o
obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
new file mode 100644
index 000..7dc32c3
--- /dev/null
+++ b/drivers/iio/adc/mcp320x.c
@@ -0,0 +1,255 @@
+/*
+ * Copyright (C) 2013 Oskar Andero oskar.and...@gmail.com
+ *
+ * Driver for Microchip Technology's MCP3204 and MCP3208 ADC chips.
+ * Datasheet can be found here:
+ * http://ww1.microchip.com/downloads/en/devicedoc/21298c.pdf
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/err.h
+#include linux/spi/spi.h
+#include linux/module.h
+#include linux/iio/iio.h
+#include linux/regulator/consumer.h
+
+#define MCP_SINGLE_ENDED (1 3)
+#define MCP_START_BIT (1 4)
+
+enum {
+ mcp3204,
+ mcp3208,
+};
+
+struct mcp320x {
+ struct spi_device *spi;
+ struct spi_message msg;
+ struct spi_transfer transfer[2];
+
+ u8 tx_buf;
+ u8 rx_buf[2];
+
+ struct regulator *reg;
+ unsigned int ref_mv;
+
+ struct mutex lock;
+};
+
+static int mcp320x_adc_conversion(struct mcp320x *adc, u8 msg)
+{
+ int ret;
+
+ adc-tx_buf = msg;
+ ret = spi_sync(adc-spi, adc-msg);
+ if (ret 0)
+ return ret;
+
+ return ((adc-rx_buf[0] 0x3f) 6) |
+ (adc-rx_buf[1] 2);
+}
+
+static int mcp320x_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *channel, int *val,
+ int *val2, long mask)
+{
+ struct mcp320x *adc = iio_priv(indio_dev);
+ int ret = -EINVAL;
+
+ mutex_lock(adc-lock);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ if (channel-differential)
+ ret = mcp320x_adc_conversion(adc,
+ MCP_START_BIT | channel-address);
+ else
+ ret = mcp320x_adc_conversion(adc,
+ MCP_START_BIT | MCP_SINGLE_ENDED |
+ channel-address);
+ if (ret 0)
+ goto out;
+
+ *val = ret;
+ ret = IIO_VAL_INT;
+ break;
+
+ case IIO_CHAN_INFO_SCALE:
+ /* Digital output code = (4096 * Vin) / Vref */
+ ret = regulator_get_voltage(adc-reg);
+ if (ret 0)
+ goto out;
+
+ *val = ret / 1000;
+ *val2 = 4096;
+ ret = IIO_VAL_FRACTIONAL;
+ break;
+
+ default:
+ break;
+ }
+
+out:
+ mutex_unlock(adc-lock);
+
+ return ret;
+}
+
+#define MCP320X_VOLTAGE_CHANNEL(num) \
+ { \
+ .type = IIO_VOLTAGE,\
+ .indexed = 1, \
+ .channel = (num), \
+ .address = (num), \
+ .info_mask_separate = BIT
Re: [PATCH] iio: adc: add driver for MCP3204/08 12-bit ADC
Hi,
On 16:07 Wed 01 May , Lars-Peter Clausen wrote:
> On 05/01/2013 12:21 AM, Oskar Andero wrote:
> > This adds support for Microchip's 12 bit AD converters MCP3204 and
> > MCP3208. These chips communicates over SPI and supports single-ended
> > and pseudo-differential configurations.
> >
> > Cc: Jonathan Cameron
> > Cc: Lars-Peter Clausen
> > Signed-off-by: Oskar Andero
>
> Hi,
>
> Looks very good in general. A few minor things, mostly related to the
> regulator
> handling and a couple of nitpicks inline.
Thanks for reviewing! I will prepare version 2 shortly.
> > +
> > + ret = regulator_enable(adc->reg);
> > + if (ret < 0)
> > + goto reg_free;
> > + } else {
> > + adc->ref_mv = pdata->ref_mv;
>
> I'd like to see this fallback path go away. For supplies with a const voltage
> the fixed-voltage-regulator can be used.
>
Ok. I added this since the voltage I use for reference is behind a
level-shifter,
meaning there is actually no regulator on the board providing that exact
voltage level. Not sure what you mean with "the fixed-voltage-regulator", but
maybe that is what I am looking for in my case?
Thanks!
-Oskar
--
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] iio: adc: add driver for MCP3204/08 12-bit ADC
Hi,
On 16:07 Wed 01 May , Lars-Peter Clausen wrote:
On 05/01/2013 12:21 AM, Oskar Andero wrote:
This adds support for Microchip's 12 bit AD converters MCP3204 and
MCP3208. These chips communicates over SPI and supports single-ended
and pseudo-differential configurations.
Cc: Jonathan Cameron ji...@cam.ac.uk
Cc: Lars-Peter Clausen l...@metafoo.de
Signed-off-by: Oskar Andero oskar.and...@gmail.com
Hi,
Looks very good in general. A few minor things, mostly related to the
regulator
handling and a couple of nitpicks inline.
Thanks for reviewing! I will prepare version 2 shortly.
+
+ ret = regulator_enable(adc-reg);
+ if (ret 0)
+ goto reg_free;
+ } else {
+ adc-ref_mv = pdata-ref_mv;
I'd like to see this fallback path go away. For supplies with a const voltage
the fixed-voltage-regulator can be used.
Ok. I added this since the voltage I use for reference is behind a
level-shifter,
meaning there is actually no regulator on the board providing that exact
voltage level. Not sure what you mean with the fixed-voltage-regulator, but
maybe that is what I am looking for in my case?
Thanks!
-Oskar
--
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] iio: adc: add driver for MCP3204/08 12-bit ADC
This adds support for Microchip's 12 bit AD converters MCP3204 and
MCP3208. These chips communicates over SPI and supports single-ended
and pseudo-differential configurations.
Cc: Jonathan Cameron
Cc: Lars-Peter Clausen
Signed-off-by: Oskar Andero
---
drivers/iio/adc/Kconfig | 10 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/mcp320x.c | 261 ++
include/linux/platform_data/mcp320x.h | 23 +++
4 files changed, 295 insertions(+)
create mode 100644 drivers/iio/adc/mcp320x.c
create mode 100644 include/linux/platform_data/mcp320x.h
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ab0767e6..93129ec 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -133,6 +133,16 @@ config MAX1363
max11646, max11647) Provides direct access via sysfs and buffered
data via the iio dev interface.
+config MCP320X
+ tristate "Microchip Technology MCP3204/08"
+ depends on SPI
+ help
+ Say yes here to build support for Microchip Technology's MCP3204 or
+ MCP3208 analog to digital converter.
+
+ This driver can also be built as a module. If so, the module will be
+ called mcp320x.
+
config TI_ADC081C
tristate "Texas Instruments ADC081C021/027"
depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 0a825be..8f475d3 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_AT91_ADC) += at91_adc.o
obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
obj-$(CONFIG_MAX1363) += max1363.o
+obj-$(CONFIG_MCP320X) += mcp320x.o
obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
new file mode 100644
index 000..cb308e8
--- /dev/null
+++ b/drivers/iio/adc/mcp320x.c
@@ -0,0 +1,261 @@
+/*
+ * Copyright (C) 2013 Oskar Andero
+ *
+ * Driver for Microchip Technology's MCP3204 and MCP3208 ADC chips.
+ * Datasheet can be found here:
+ * http://ww1.microchip.com/downloads/en/devicedoc/21298c.pdf
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define MCP_SINGLE_ENDED (1 << 3)
+#define MCP_START_BIT (1 << 4)
+
+enum {
+ mcp3204,
+ mcp3208,
+};
+
+struct mcp320x {
+ struct spi_device *spi;
+ struct spi_message msg;
+ struct spi_transfer transfer[2];
+
+ u8 tx_buf;
+ u8 rx_buf[2];
+
+ struct regulator *reg;
+ unsigned int ref_mv;
+
+ struct mutex lock;
+};
+
+static int mcp320x_adc_conversion(struct mcp320x *adc, u8 msg)
+{
+ int ret;
+
+ adc->tx_buf = msg;
+ ret = spi_sync(adc->spi, >msg);
+ if (ret < 0)
+ return ret;
+
+ return ((adc->rx_buf[0] & 0x3f) << 6) |
+ (adc->rx_buf[1] >> 2);
+}
+
+static int mcp320x_read_raw(struct iio_dev *iio,
+ struct iio_chan_spec const *channel, int *val,
+ int *val2, long mask)
+{
+ struct mcp320x *adc = iio_priv(iio);
+ int ret = -EINVAL;
+
+ mutex_lock(>lock);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ if (channel->differential)
+ ret = mcp320x_adc_conversion(adc,
+ MCP_START_BIT | channel->address);
+ else
+ ret = mcp320x_adc_conversion(adc,
+ MCP_START_BIT | MCP_SINGLE_ENDED |
+ channel->address);
+ if (ret < 0)
+ goto out;
+
+ *val = ret;
+ ret = IIO_VAL_INT;
+ break;
+
+ case IIO_CHAN_INFO_SCALE:
+ /* Digital output code = (4096 * Vin) / Vref */
+ if (!IS_ERR_OR_NULL(adc->reg)) {
+ ret = regulator_get_voltage(adc->reg);
+ if (ret < 0)
+ goto out;
+ *val = ret / 1000;
+ } else {
+ *val = adc->ref_mv;
+ }
+ *val2 = 4096 * 1000;
+ ret = IIO_VAL_FRACTIONAL;
+ break;
+
+ default:
+ break;
+ }
+
+out:
+ mutex_unlock(>lock);
+
+ return ret;
+}
+
+#define MCP320X_VOLTAGE_CHANNEL(num) \
+ { \
+ .type = IIO_VOLTAGE,
[PATCH] iio: adc: add driver for MCP3204/08 12-bit ADC
This adds support for Microchip's 12 bit AD converters MCP3204 and
MCP3208. These chips communicates over SPI and supports single-ended
and pseudo-differential configurations.
Cc: Jonathan Cameron ji...@cam.ac.uk
Cc: Lars-Peter Clausen l...@metafoo.de
Signed-off-by: Oskar Andero oskar.and...@gmail.com
---
drivers/iio/adc/Kconfig | 10 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/mcp320x.c | 261 ++
include/linux/platform_data/mcp320x.h | 23 +++
4 files changed, 295 insertions(+)
create mode 100644 drivers/iio/adc/mcp320x.c
create mode 100644 include/linux/platform_data/mcp320x.h
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ab0767e6..93129ec 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -133,6 +133,16 @@ config MAX1363
max11646, max11647) Provides direct access via sysfs and buffered
data via the iio dev interface.
+config MCP320X
+ tristate Microchip Technology MCP3204/08
+ depends on SPI
+ help
+ Say yes here to build support for Microchip Technology's MCP3204 or
+ MCP3208 analog to digital converter.
+
+ This driver can also be built as a module. If so, the module will be
+ called mcp320x.
+
config TI_ADC081C
tristate Texas Instruments ADC081C021/027
depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 0a825be..8f475d3 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_AT91_ADC) += at91_adc.o
obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
obj-$(CONFIG_MAX1363) += max1363.o
+obj-$(CONFIG_MCP320X) += mcp320x.o
obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
new file mode 100644
index 000..cb308e8
--- /dev/null
+++ b/drivers/iio/adc/mcp320x.c
@@ -0,0 +1,261 @@
+/*
+ * Copyright (C) 2013 Oskar Andero oskar.and...@gmail.com
+ *
+ * Driver for Microchip Technology's MCP3204 and MCP3208 ADC chips.
+ * Datasheet can be found here:
+ * http://ww1.microchip.com/downloads/en/devicedoc/21298c.pdf
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/err.h
+#include linux/spi/spi.h
+#include linux/module.h
+#include linux/iio/iio.h
+#include linux/regulator/consumer.h
+#include linux/platform_data/mcp320x.h
+
+#define MCP_SINGLE_ENDED (1 3)
+#define MCP_START_BIT (1 4)
+
+enum {
+ mcp3204,
+ mcp3208,
+};
+
+struct mcp320x {
+ struct spi_device *spi;
+ struct spi_message msg;
+ struct spi_transfer transfer[2];
+
+ u8 tx_buf;
+ u8 rx_buf[2];
+
+ struct regulator *reg;
+ unsigned int ref_mv;
+
+ struct mutex lock;
+};
+
+static int mcp320x_adc_conversion(struct mcp320x *adc, u8 msg)
+{
+ int ret;
+
+ adc-tx_buf = msg;
+ ret = spi_sync(adc-spi, adc-msg);
+ if (ret 0)
+ return ret;
+
+ return ((adc-rx_buf[0] 0x3f) 6) |
+ (adc-rx_buf[1] 2);
+}
+
+static int mcp320x_read_raw(struct iio_dev *iio,
+ struct iio_chan_spec const *channel, int *val,
+ int *val2, long mask)
+{
+ struct mcp320x *adc = iio_priv(iio);
+ int ret = -EINVAL;
+
+ mutex_lock(adc-lock);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ if (channel-differential)
+ ret = mcp320x_adc_conversion(adc,
+ MCP_START_BIT | channel-address);
+ else
+ ret = mcp320x_adc_conversion(adc,
+ MCP_START_BIT | MCP_SINGLE_ENDED |
+ channel-address);
+ if (ret 0)
+ goto out;
+
+ *val = ret;
+ ret = IIO_VAL_INT;
+ break;
+
+ case IIO_CHAN_INFO_SCALE:
+ /* Digital output code = (4096 * Vin) / Vref */
+ if (!IS_ERR_OR_NULL(adc-reg)) {
+ ret = regulator_get_voltage(adc-reg);
+ if (ret 0)
+ goto out;
+ *val = ret / 1000;
+ } else {
+ *val = adc-ref_mv;
+ }
+ *val2 = 4096 * 1000;
+ ret = IIO_VAL_FRACTIONAL;
+ break;
+
+ default:
+ break;
+ }
+
+out:
+ mutex_unlock(adc-lock);
+
+ return ret;
+}
+
+#define MCP320X_VOLTAGE_CHANNEL(num
Re: [PATCH] lowmemorykiller: prevent multiple instances of low memory killer
On 22:00 Tue 16 Apr , David Rientjes wrote:
> On Tue, 16 Apr 2013, Oskar Andero wrote:
>
> > > > The comment in shrinker.h is misleading, not the source code.
> > > > do_shrinker_shrink() will fail for anything negative and 0.
> > >
> > > The comment is correct. The only acceptable negative return is -1.
> > > Look at the second time do_shrinker_shrink() is called from
> > > shrink_slab().
> > >
> > >283 while (total_scan >= batch_size) {
> > >284 int nr_before;
> > >285
> > >286 nr_before = do_shrinker_shrink(shrinker,
> > > shrink, 0);
> > >287 shrink_ret = do_shrinker_shrink(shrinker,
> > > shrink,
> > >288
> > > batch_size);
> > >289 if (shrink_ret == -1)
> > >290 break;
> > >291 if (shrink_ret < nr_before)
> > >292 ret += nr_before - shrink_ret;
> > >293 count_vm_events(SLABS_SCANNED,
> > > batch_size);
> >
> > Yes, the comment is correct with what is implemented in the code, but
> > that doesn't mean the code is right. IMHO, relaying on magical numbers is
> > highly
> > questionable coding style.
> >
>
> No, it's not. This is controlled higher in shrink_slab() by this:
>
> max_pass = do_shrinker_shrink(shrinker, shrink, 0);
> if (max_pass <= 0)
> continue;
>
Sure, that looks ok, but that doesn't change the fact that line 289
above has a magical number and I guess that explains the comment:
> > >289 if (shrink_ret == -1)
> > >290 break;
Just to be clear - this is not about lowmemkiller, but rather a generic
clean-up of shrinkers that is needed IMO.
> and your patch is implemented incorrectly, i.e. it does not return
> LMK_BUSY if the spinlock is contended which needlessly recalls the
> shrinker later.
>
> You have a couple of options:
>
> - return -1 when the spinlock is contended immediately when
>!sc->nr_to_scan (although it should really be a cmpxchg since a
>spinlock isn't needed), or
I leave it to Snild to comment on the patch, but could you elaborate on why
you think cmpxchg is a better alternative than a spin_trylock? I just had a
brief look at the implementation for ARM and it looks like cmpxchg means
two unconditional memory barriers, whereas spin_trylock has one
conditional memory barrier. See arch/arm/include/asm/spinlock.h:
if (tmp == 0) {
smp_mb();
return 1;
} else {
return 0;
}
...and arch/arm/include/asm/cmpxchg.h:
smp_mb();
ret = __cmpxchg(ptr, old, new, size);
smp_mb();
AFAIK a memory barrier is pretty costly.
-Oskar
--
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] lowmemorykiller: prevent multiple instances of low memory killer
On 22:00 Tue 16 Apr , David Rientjes wrote:
On Tue, 16 Apr 2013, Oskar Andero wrote:
The comment in shrinker.h is misleading, not the source code.
do_shrinker_shrink() will fail for anything negative and 0.
The comment is correct. The only acceptable negative return is -1.
Look at the second time do_shrinker_shrink() is called from
shrink_slab().
283 while (total_scan = batch_size) {
284 int nr_before;
285
286 nr_before = do_shrinker_shrink(shrinker,
shrink, 0);
287 shrink_ret = do_shrinker_shrink(shrinker,
shrink,
288
batch_size);
289 if (shrink_ret == -1)
290 break;
291 if (shrink_ret nr_before)
292 ret += nr_before - shrink_ret;
293 count_vm_events(SLABS_SCANNED,
batch_size);
Yes, the comment is correct with what is implemented in the code, but
that doesn't mean the code is right. IMHO, relaying on magical numbers is
highly
questionable coding style.
No, it's not. This is controlled higher in shrink_slab() by this:
max_pass = do_shrinker_shrink(shrinker, shrink, 0);
if (max_pass = 0)
continue;
Sure, that looks ok, but that doesn't change the fact that line 289
above has a magical number and I guess that explains the comment:
289 if (shrink_ret == -1)
290 break;
Just to be clear - this is not about lowmemkiller, but rather a generic
clean-up of shrinkers that is needed IMO.
and your patch is implemented incorrectly, i.e. it does not return
LMK_BUSY if the spinlock is contended which needlessly recalls the
shrinker later.
You have a couple of options:
- return -1 when the spinlock is contended immediately when
!sc-nr_to_scan (although it should really be a cmpxchg since a
spinlock isn't needed), or
I leave it to Snild to comment on the patch, but could you elaborate on why
you think cmpxchg is a better alternative than a spin_trylock? I just had a
brief look at the implementation for ARM and it looks like cmpxchg means
two unconditional memory barriers, whereas spin_trylock has one
conditional memory barrier. See arch/arm/include/asm/spinlock.h:
if (tmp == 0) {
smp_mb();
return 1;
} else {
return 0;
}
...and arch/arm/include/asm/cmpxchg.h:
smp_mb();
ret = __cmpxchg(ptr, old, new, size);
smp_mb();
AFAIK a memory barrier is pretty costly.
-Oskar
--
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] lowmemorykiller: prevent multiple instances of low memory killer
On 08:19 Tue 16 Apr , Dan Carpenter wrote:
> On Mon, Apr 15, 2013 at 04:11:18PM -0700, David Rientjes wrote:
> > On Mon, 15 Apr 2013, Greg Kroah-Hartman wrote:
> >
> > > > The positive numbers are used to return information on the remaining
> > > > cache size (again, see the comment I pasted above). We could use
> > > > -EBUSY, but we'd have to change vmscan.c, which checks specifically
> > > > for -1. I can't see a technical reason why -EBUSY couldn't have been
> > > > chosen instead, but there's also no real reason to change it now.
> > >
> > > If it's not the correct thing to do, sure we can change it, just send a
> > > patch. It makes way more sense than some random -1 return value to me.
> > >
> > > Care to send a series of patches fixing this up properly?
> > >
> >
> > The comment in shrinker.h is misleading, not the source code.
> > do_shrinker_shrink() will fail for anything negative and 0.
>
> The comment is correct. The only acceptable negative return is -1.
> Look at the second time do_shrinker_shrink() is called from
> shrink_slab().
>
>283 while (total_scan >= batch_size) {
>284 int nr_before;
>285
>286 nr_before = do_shrinker_shrink(shrinker,
> shrink, 0);
>287 shrink_ret = do_shrinker_shrink(shrinker,
> shrink,
>288 batch_size);
>289 if (shrink_ret == -1)
>290 break;
>291 if (shrink_ret < nr_before)
>292 ret += nr_before - shrink_ret;
>293 count_vm_events(SLABS_SCANNED, batch_size);
Yes, the comment is correct with what is implemented in the code, but
that doesn't mean the code is right. IMHO, relaying on magical numbers is highly
questionable coding style.
If there are no objections I will prepare a patch-set and let's discuss it from
there.
-Oskar
--
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] lowmemorykiller: prevent multiple instances of low memory killer
On 08:19 Tue 16 Apr , Dan Carpenter wrote:
On Mon, Apr 15, 2013 at 04:11:18PM -0700, David Rientjes wrote:
On Mon, 15 Apr 2013, Greg Kroah-Hartman wrote:
The positive numbers are used to return information on the remaining
cache size (again, see the comment I pasted above). We could use
-EBUSY, but we'd have to change vmscan.c, which checks specifically
for -1. I can't see a technical reason why -EBUSY couldn't have been
chosen instead, but there's also no real reason to change it now.
If it's not the correct thing to do, sure we can change it, just send a
patch. It makes way more sense than some random -1 return value to me.
Care to send a series of patches fixing this up properly?
The comment in shrinker.h is misleading, not the source code.
do_shrinker_shrink() will fail for anything negative and 0.
The comment is correct. The only acceptable negative return is -1.
Look at the second time do_shrinker_shrink() is called from
shrink_slab().
283 while (total_scan = batch_size) {
284 int nr_before;
285
286 nr_before = do_shrinker_shrink(shrinker,
shrink, 0);
287 shrink_ret = do_shrinker_shrink(shrinker,
shrink,
288 batch_size);
289 if (shrink_ret == -1)
290 break;
291 if (shrink_ret nr_before)
292 ret += nr_before - shrink_ret;
293 count_vm_events(SLABS_SCANNED, batch_size);
Yes, the comment is correct with what is implemented in the code, but
that doesn't mean the code is right. IMHO, relaying on magical numbers is highly
questionable coding style.
If there are no objections I will prepare a patch-set and let's discuss it from
there.
-Oskar
--
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] lowmemorykiller: prevent multiple instances of low memory killer
On 16:13 Mon 15 Apr , Dan Carpenter wrote: > On Mon, Apr 15, 2013 at 03:38:08PM +0200, Dolkow, Snild wrote: > > >Where is lowmem_shrink called from? I only see shrink called from the > > >bcache sysfs handler __bch_cache_set(). The return value isn't checked > > >there. > > > > > >Up to now this function has only returns positive numbers. > > > > > >There isn't a place which check LMK_BUSY so maybe it's best to just > > >return zero? > > > > Hey Dan, > > > > lowmem_shrink is assigned to a shrinker struct > > (include/linux/shrinker.h) and called in do_shrinker_shrink() in > > mm/vmscan.c. That, in turn, is called and checked in a few places > > in vmscan.c. > > > > >From the comments in shrinker.h: > > "It should return the number of objects which remain in the > > cache. If it returns -1, it means it cannot do any scanning at > > this time (eg. there is a risk of deadlock). The callback must not > > return -1 if nr_to_scan is zero." > > Ah. Good. -1 is the right return. > > But really should be a #define in shrinker.h instead of in > drivers/staging/android/. IMO one should use the errno.h values - e.g. EBUSY might be a good value in this case. Does anyone know why the shrinker wants -1? Is there a reason? -Oskar -- 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] lowmemorykiller: prevent multiple instances of low memory killer
From: Snild Dolkow
Running multiple instances of LMK is not useful since it will try to
kill the same process.
This patch adds a spinlock to prevent multiple instances of the LMK
running at the same time. Uses spin_trylock and return on failure to
avoid blocking.
Cc: Greg Kroah-Hartman
Cc: Brian Swetland
Reviewed-by: Radovan Lekanovic
Signed-off-by: Snild Dolkow
Signed-off-by: Oskar Andero
---
drivers/staging/android/lowmemorykiller.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/staging/android/lowmemorykiller.c
b/drivers/staging/android/lowmemorykiller.c
index 3b91b0f..0b19353 100644
--- a/drivers/staging/android/lowmemorykiller.c
+++ b/drivers/staging/android/lowmemorykiller.c
@@ -38,6 +38,7 @@
#include
#include
#include
+#include
static uint32_t lowmem_debug_level = 2;
static short lowmem_adj[6] = {
@@ -57,6 +58,8 @@ static int lowmem_minfree_size = 4;
static unsigned long lowmem_deathpending_timeout;
+#define LMK_BUSY (-1)
+
#define lowmem_print(level, x...) \
do {\
if (lowmem_debug_level >= (level)) \
@@ -65,6 +68,7 @@ static unsigned long lowmem_deathpending_timeout;
static int lowmem_shrink(struct shrinker *s, struct shrink_control *sc)
{
+ static DEFINE_SPINLOCK(lowmem_lock);
struct task_struct *tsk;
struct task_struct *selected = NULL;
int rem = 0;
@@ -104,6 +108,9 @@ static int lowmem_shrink(struct shrinker *s, struct
shrink_control *sc)
}
selected_oom_score_adj = min_score_adj;
+ if (spin_trylock(_lock) == 0)
+ return LMK_BUSY;
+
rcu_read_lock();
for_each_process(tsk) {
struct task_struct *p;
@@ -120,6 +127,7 @@ static int lowmem_shrink(struct shrinker *s, struct
shrink_control *sc)
time_before_eq(jiffies, lowmem_deathpending_timeout)) {
task_unlock(p);
rcu_read_unlock();
+ spin_unlock(_lock);
return 0;
}
oom_score_adj = p->signal->oom_score_adj;
@@ -156,6 +164,7 @@ static int lowmem_shrink(struct shrinker *s, struct
shrink_control *sc)
lowmem_print(4, "lowmem_shrink %lu, %x, return %d\n",
sc->nr_to_scan, sc->gfp_mask, rem);
rcu_read_unlock();
+ spin_unlock(_lock);
return rem;
}
--
1.8.1.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] lowmemorykiller: prevent multiple instances of low memory killer
From: Snild Dolkow snild.dol...@sonymobile.com
Running multiple instances of LMK is not useful since it will try to
kill the same process.
This patch adds a spinlock to prevent multiple instances of the LMK
running at the same time. Uses spin_trylock and return on failure to
avoid blocking.
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Cc: Brian Swetland swetl...@google.com
Reviewed-by: Radovan Lekanovic radovan.lekano...@sonymobile.com
Signed-off-by: Snild Dolkow snild.dol...@sonymobile.com
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
---
drivers/staging/android/lowmemorykiller.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/staging/android/lowmemorykiller.c
b/drivers/staging/android/lowmemorykiller.c
index 3b91b0f..0b19353 100644
--- a/drivers/staging/android/lowmemorykiller.c
+++ b/drivers/staging/android/lowmemorykiller.c
@@ -38,6 +38,7 @@
#include linux/rcupdate.h
#include linux/profile.h
#include linux/notifier.h
+#include linux/spinlock.h
static uint32_t lowmem_debug_level = 2;
static short lowmem_adj[6] = {
@@ -57,6 +58,8 @@ static int lowmem_minfree_size = 4;
static unsigned long lowmem_deathpending_timeout;
+#define LMK_BUSY (-1)
+
#define lowmem_print(level, x...) \
do {\
if (lowmem_debug_level = (level)) \
@@ -65,6 +68,7 @@ static unsigned long lowmem_deathpending_timeout;
static int lowmem_shrink(struct shrinker *s, struct shrink_control *sc)
{
+ static DEFINE_SPINLOCK(lowmem_lock);
struct task_struct *tsk;
struct task_struct *selected = NULL;
int rem = 0;
@@ -104,6 +108,9 @@ static int lowmem_shrink(struct shrinker *s, struct
shrink_control *sc)
}
selected_oom_score_adj = min_score_adj;
+ if (spin_trylock(lowmem_lock) == 0)
+ return LMK_BUSY;
+
rcu_read_lock();
for_each_process(tsk) {
struct task_struct *p;
@@ -120,6 +127,7 @@ static int lowmem_shrink(struct shrinker *s, struct
shrink_control *sc)
time_before_eq(jiffies, lowmem_deathpending_timeout)) {
task_unlock(p);
rcu_read_unlock();
+ spin_unlock(lowmem_lock);
return 0;
}
oom_score_adj = p-signal-oom_score_adj;
@@ -156,6 +164,7 @@ static int lowmem_shrink(struct shrinker *s, struct
shrink_control *sc)
lowmem_print(4, lowmem_shrink %lu, %x, return %d\n,
sc-nr_to_scan, sc-gfp_mask, rem);
rcu_read_unlock();
+ spin_unlock(lowmem_lock);
return rem;
}
--
1.8.1.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] lowmemorykiller: prevent multiple instances of low memory killer
On 16:13 Mon 15 Apr , Dan Carpenter wrote: On Mon, Apr 15, 2013 at 03:38:08PM +0200, Dolkow, Snild wrote: Where is lowmem_shrink called from? I only see shrink called from the bcache sysfs handler __bch_cache_set(). The return value isn't checked there. Up to now this function has only returns positive numbers. There isn't a place which check LMK_BUSY so maybe it's best to just return zero? Hey Dan, lowmem_shrink is assigned to a shrinker struct (include/linux/shrinker.h) and called in do_shrinker_shrink() in mm/vmscan.c. That, in turn, is called and checked in a few places in vmscan.c. From the comments in shrinker.h: It should return the number of objects which remain in the cache. If it returns -1, it means it cannot do any scanning at this time (eg. there is a risk of deadlock). The callback must not return -1 if nr_to_scan is zero. Ah. Good. -1 is the right return. But really should be a #define in shrinker.h instead of in drivers/staging/android/. IMO one should use the errno.h values - e.g. EBUSY might be a good value in this case. Does anyone know why the shrinker wants -1? Is there a reason? -Oskar -- 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] scsi: fix null pointer dereference in sd_revalidate_disk
Hi James,
On 18:39 Mon 08 Apr , James Bottomley wrote:
> On Mon, 2013-04-08 at 13:42 +0200, Oskar Andero wrote:
> > On 14:36 Thu 07 Mar , oskar.and...@sonymobile.com wrote:
> > > From: "syunsuke.x.itou"
> > >
> > > By repeatadly connecting/disconnecting a USB masstorage device
> > > a null pointer dereference can be provoked. This is caused by
> > > sd_revalidate_disk() assuming sdkp to be a valid pointer. This
> > > patch adds a null pointer check.
> > >
> > > Reviewed-by: Radovan Lekanovic
> > > Signed-off-by: syunsuke.x.itou
> > > Signed-off-by: Oskar Andero
> > > ---
> > > drivers/scsi/sd.c |7 ++-
> > > 1 files changed, 6 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > > index 7992635..49849ce 100644
> > > --- a/drivers/scsi/sd.c
> > > +++ b/drivers/scsi/sd.c
> > > @@ -1,6 +1,7 @@
> > > /*
> > > * sd.c Copyright (C) 1992 Drew Eckhardt
> > > * Copyright (C) 1993, 1994, 1995, 1999 Eric Youngdale
> > > + * Copyright (C) 2013 Sony Mobile Communications AB.
> > > *
> > > * Linux scsi disk driver
> > > * Initial versions: Drew Eckhardt
> > > @@ -2653,10 +2654,14 @@ static int sd_try_extended_inquiry(struct
> > > scsi_device *sdp)
> > > static int sd_revalidate_disk(struct gendisk *disk)
> > > {
> > > struct scsi_disk *sdkp = scsi_disk(disk);
> > > - struct scsi_device *sdp = sdkp->device;
> > > + struct scsi_device *sdp;
> > > unsigned char *buffer;
> > > unsigned flush = 0;
> > >
> > > + if (!sdkp)
> > > + return -ENODEV;
> > > + sdp = sdkp->device;
> > > +
> > > SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp,
> > > "sd_revalidate_disk\n"));
> > >
> > > --
> > > 1.7.8.6
> > >
> >
> > Ping. Any input on the patch above?
>
> For us to give input, the original patch would have to have been sent to
> linux-scsi ...
That's strange. Must have been lost in our mail servers. It explains
why I didn't find the patch in the archives though. :)
> However, I think it's not correct because it fixes a symptom, not the
> cause. If sdkp can be null, that means something failed to get the
> reference that's required. What's the stack trace?
This is the stack trace:
[...]
[ 1896.454417] Unable to handle kernel NULL pointer dereference at virtual
address 0004
[ 1896.461834] pgd = d7b2
[ 1896.464245] [0004] *pgd=85bf6831, *pte=, *ppte=
[ 1896.470471] Internal error: Oops: 17 [#1] PREEMPT SMP
[ 1896.475537] Modules linked in:
[ 1896.476392] hub 1-0:1.0: hub_port_status failed (err = -19)
[ 1896.476422] hub 1-0:1.0: connect-debounce failed, port 1 disabled
[ 1896.490248]
[ 1896.490279] CPU: 1Tainted: GW
(3.0.21-perf-gd3cbbb2-00351-gbcb8998 #1)
[ 1896.490279] PC is at sd_revalidate_disk+0x1c/0x1720
[ 1896.490309] LR is at rescan_partitions+0xb0/0x498
[ 1896.490309] pc : []lr : []psr: a013
[ 1896.490309] sp : d7b25c68 ip : 0003 fp : 0001
[ 1896.490309] r10: e0822a00 r9 : r8 : dce93c0c
[ 1896.490309] r7 : 001f r6 : dce93c00 r5 : dce93c00 r4 :
[ 1896.490340] r3 : 271aed09 r2 : d7b25cd8 r1 : r0 : dce93c00
[ 1896.490340] Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment
user
[ 1896.490340] Control: 10c5787d Table: 9ef2006a DAC: 0015
[...]
[ 1896.492049] [] (sd_revalidate_disk+0x1c/0x1720) from []
(rescan_partitions+0xb0/0x498)
[ 1896.492049] [] (rescan_partitions+0xb0/0x498) from []
(__blkdev_get+0x154/0x320)
[ 1896.492079] [] (__blkdev_get+0x154/0x320) from []
(__blkdev_get+0x194/0x320)
[ 1896.492079] [] (__blkdev_get+0x194/0x320) from []
(blkdev_get+0x1d8/0x330)
[ 1896.492110] [] (blkdev_get+0x1d8/0x330) from []
(__dentry_open+0x220/0x338)
[ 1896.492110] [] (__dentry_open+0x220/0x338) from []
(nameidata_to_filp+0x50/0x5c)
[ 1896.492140] [] (nameidata_to_filp+0x50/0x5c) from []
(do_last+0x7c8/0x8d4)
[ 1896.492140] [] (do_last+0x7c8/0x8d4) from []
(path_openat+0xc4/0x380)
[ 1896.492171] [] (path_openat+0xc4/0x380) from []
(do_filp_open+0x30/0x7c)
[ 1896.492171] [] (do_filp_open+0x30/0x7c) from []
(do_sys_open+0xd8/0x174)
[ 1896.492201] [] (do_sys_open+0xd8/0x174) from []
(ret_fast_syscall+0x0/0x30)
[ 1896.492201] Code: e5933000 e58d0024 e59041f0 e58d305c (e5940004)
The patch above fixes the problem, but you might be right - maybe it only fixes
the symptom.
Let me know if you have any suggestions or need more information!
-Oskar
--
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] scsi: fix null pointer dereference in sd_revalidate_disk
Hi James,
On 18:39 Mon 08 Apr , James Bottomley wrote:
On Mon, 2013-04-08 at 13:42 +0200, Oskar Andero wrote:
On 14:36 Thu 07 Mar , oskar.and...@sonymobile.com wrote:
From: syunsuke.x.itou syunsuke.x.i...@sonymobile.com
By repeatadly connecting/disconnecting a USB masstorage device
a null pointer dereference can be provoked. This is caused by
sd_revalidate_disk() assuming sdkp to be a valid pointer. This
patch adds a null pointer check.
Reviewed-by: Radovan Lekanovic radovan.lekano...@sonymobile.com
Signed-off-by: syunsuke.x.itou syunsuke.x.i...@sonymobile.com
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
---
drivers/scsi/sd.c |7 ++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 7992635..49849ce 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1,6 +1,7 @@
/*
* sd.c Copyright (C) 1992 Drew Eckhardt
* Copyright (C) 1993, 1994, 1995, 1999 Eric Youngdale
+ * Copyright (C) 2013 Sony Mobile Communications AB.
*
* Linux scsi disk driver
* Initial versions: Drew Eckhardt
@@ -2653,10 +2654,14 @@ static int sd_try_extended_inquiry(struct
scsi_device *sdp)
static int sd_revalidate_disk(struct gendisk *disk)
{
struct scsi_disk *sdkp = scsi_disk(disk);
- struct scsi_device *sdp = sdkp-device;
+ struct scsi_device *sdp;
unsigned char *buffer;
unsigned flush = 0;
+ if (!sdkp)
+ return -ENODEV;
+ sdp = sdkp-device;
+
SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp,
sd_revalidate_disk\n));
--
1.7.8.6
Ping. Any input on the patch above?
For us to give input, the original patch would have to have been sent to
linux-scsi ...
That's strange. Must have been lost in our mail servers. It explains
why I didn't find the patch in the archives though. :)
However, I think it's not correct because it fixes a symptom, not the
cause. If sdkp can be null, that means something failed to get the
reference that's required. What's the stack trace?
This is the stack trace:
[...]
[ 1896.454417] Unable to handle kernel NULL pointer dereference at virtual
address 0004
[ 1896.461834] pgd = d7b2
[ 1896.464245] [0004] *pgd=85bf6831, *pte=, *ppte=
[ 1896.470471] Internal error: Oops: 17 [#1] PREEMPT SMP
[ 1896.475537] Modules linked in:
[ 1896.476392] hub 1-0:1.0: hub_port_status failed (err = -19)
[ 1896.476422] hub 1-0:1.0: connect-debounce failed, port 1 disabled
[ 1896.490248]
[ 1896.490279] CPU: 1Tainted: GW
(3.0.21-perf-gd3cbbb2-00351-gbcb8998 #1)
[ 1896.490279] PC is at sd_revalidate_disk+0x1c/0x1720
[ 1896.490309] LR is at rescan_partitions+0xb0/0x498
[ 1896.490309] pc : [c043e7ec]lr : [c026a0e4]psr: a013
[ 1896.490309] sp : d7b25c68 ip : 0003 fp : 0001
[ 1896.490309] r10: e0822a00 r9 : r8 : dce93c0c
[ 1896.490309] r7 : 001f r6 : dce93c00 r5 : dce93c00 r4 :
[ 1896.490340] r3 : 271aed09 r2 : d7b25cd8 r1 : r0 : dce93c00
[ 1896.490340] Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment
user
[ 1896.490340] Control: 10c5787d Table: 9ef2006a DAC: 0015
[...]
[ 1896.492049] [c043e7ec] (sd_revalidate_disk+0x1c/0x1720) from [c026a0e4]
(rescan_partitions+0xb0/0x498)
[ 1896.492049] [c026a0e4] (rescan_partitions+0xb0/0x498) from [c024c648]
(__blkdev_get+0x154/0x320)
[ 1896.492079] [c024c648] (__blkdev_get+0x154/0x320) from [c024c688]
(__blkdev_get+0x194/0x320)
[ 1896.492079] [c024c688] (__blkdev_get+0x194/0x320) from [c024c9ec]
(blkdev_get+0x1d8/0x330)
[ 1896.492110] [c024c9ec] (blkdev_get+0x1d8/0x330) from [c021d03c]
(__dentry_open+0x220/0x338)
[ 1896.492110] [c021d03c] (__dentry_open+0x220/0x338) from [c021d204]
(nameidata_to_filp+0x50/0x5c)
[ 1896.492140] [c021d204] (nameidata_to_filp+0x50/0x5c) from [c022b244]
(do_last+0x7c8/0x8d4)
[ 1896.492140] [c022b244] (do_last+0x7c8/0x8d4) from [c022c12c]
(path_openat+0xc4/0x380)
[ 1896.492171] [c022c12c] (path_openat+0xc4/0x380) from [c022c4c8]
(do_filp_open+0x30/0x7c)
[ 1896.492171] [c022c4c8] (do_filp_open+0x30/0x7c) from [c021cc80]
(do_sys_open+0xd8/0x174)
[ 1896.492201] [c021cc80] (do_sys_open+0xd8/0x174) from [c0105e80]
(ret_fast_syscall+0x0/0x30)
[ 1896.492201] Code: e5933000 e58d0024 e59041f0 e58d305c (e5940004)
The patch above fixes the problem, but you might be right - maybe it only fixes
the symptom.
Let me know if you have any suggestions or need more information!
-Oskar
--
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] scsi: fix null pointer dereference in sd_revalidate_disk
On 14:36 Thu 07 Mar , oskar.and...@sonymobile.com wrote:
> From: "syunsuke.x.itou"
>
> By repeatadly connecting/disconnecting a USB masstorage device
> a null pointer dereference can be provoked. This is caused by
> sd_revalidate_disk() assuming sdkp to be a valid pointer. This
> patch adds a null pointer check.
>
> Reviewed-by: Radovan Lekanovic
> Signed-off-by: syunsuke.x.itou
> Signed-off-by: Oskar Andero
> ---
> drivers/scsi/sd.c |7 ++-
> 1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 7992635..49849ce 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1,6 +1,7 @@
> /*
> * sd.c Copyright (C) 1992 Drew Eckhardt
> * Copyright (C) 1993, 1994, 1995, 1999 Eric Youngdale
> + * Copyright (C) 2013 Sony Mobile Communications AB.
> *
> * Linux scsi disk driver
> * Initial versions: Drew Eckhardt
> @@ -2653,10 +2654,14 @@ static int sd_try_extended_inquiry(struct scsi_device
> *sdp)
> static int sd_revalidate_disk(struct gendisk *disk)
> {
> struct scsi_disk *sdkp = scsi_disk(disk);
> - struct scsi_device *sdp = sdkp->device;
> + struct scsi_device *sdp;
> unsigned char *buffer;
> unsigned flush = 0;
>
> + if (!sdkp)
> + return -ENODEV;
> + sdp = sdkp->device;
> +
> SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp,
> "sd_revalidate_disk\n"));
>
> --
> 1.7.8.6
>
Ping. Any input on the patch above?
Thanks,
-Oskar
--
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] scsi: fix null pointer dereference in sd_revalidate_disk
On 14:36 Thu 07 Mar , oskar.and...@sonymobile.com wrote:
From: syunsuke.x.itou syunsuke.x.i...@sonymobile.com
By repeatadly connecting/disconnecting a USB masstorage device
a null pointer dereference can be provoked. This is caused by
sd_revalidate_disk() assuming sdkp to be a valid pointer. This
patch adds a null pointer check.
Reviewed-by: Radovan Lekanovic radovan.lekano...@sonymobile.com
Signed-off-by: syunsuke.x.itou syunsuke.x.i...@sonymobile.com
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
---
drivers/scsi/sd.c |7 ++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 7992635..49849ce 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1,6 +1,7 @@
/*
* sd.c Copyright (C) 1992 Drew Eckhardt
* Copyright (C) 1993, 1994, 1995, 1999 Eric Youngdale
+ * Copyright (C) 2013 Sony Mobile Communications AB.
*
* Linux scsi disk driver
* Initial versions: Drew Eckhardt
@@ -2653,10 +2654,14 @@ static int sd_try_extended_inquiry(struct scsi_device
*sdp)
static int sd_revalidate_disk(struct gendisk *disk)
{
struct scsi_disk *sdkp = scsi_disk(disk);
- struct scsi_device *sdp = sdkp-device;
+ struct scsi_device *sdp;
unsigned char *buffer;
unsigned flush = 0;
+ if (!sdkp)
+ return -ENODEV;
+ sdp = sdkp-device;
+
SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp,
sd_revalidate_disk\n));
--
1.7.8.6
Ping. Any input on the patch above?
Thanks,
-Oskar
--
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 v3 1/4] kprobes: delay blacklist symbol lookup until we actually need it
From: Toby Collett
The symbol lookup can take a long time and kprobes is
initialised very early in boot, so delay symbol lookup
until the blacklist is first used.
Cc: Masami Hiramatsu
Cc: David S. Miller
Reviewed-by: Radovan Lekanovic
Signed-off-by: Toby Collett
Signed-off-by: Oskar Andero
---
kernel/kprobes.c | 100 ++-
1 file changed, 62 insertions(+), 38 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index e35be53..c8c2281 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -68,6 +68,7 @@
#endif
static int kprobes_initialized;
+static bool kprobe_blacklist_initialized;
static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
@@ -102,6 +103,62 @@ static struct kprobe_blackpoint kprobe_blacklist[] = {
{NULL}/* Terminator */
};
+/* it can take some time ( > 100ms ) to initialise the
+ * blacklist so we delay this until we actually need it
+ */
+static void init_kprobe_blacklist(void)
+{
+ int i;
+ unsigned long offset = 0, size = 0;
+ char *modname, namebuf[128];
+ const char *symbol_name;
+ void *addr;
+ struct kprobe_blackpoint *kb;
+
+ mutex_lock(_mutex);
+ if (kprobe_blacklist_initialized)
+ goto out;
+
+ /*
+* Lookup and populate the kprobe_blacklist.
+*
+* Unlike the kretprobe blacklist, we'll need to determine
+* the range of addresses that belong to the said functions,
+* since a kprobe need not necessarily be at the beginning
+* of a function.
+*/
+ for (kb = kprobe_blacklist; kb->name != NULL; kb++) {
+ kprobe_lookup_name(kb->name, addr);
+ if (!addr)
+ continue;
+
+ kb->start_addr = (unsigned long)addr;
+ symbol_name = kallsyms_lookup(kb->start_addr,
+ , , , namebuf);
+ if (!symbol_name)
+ kb->range = 0;
+ else
+ kb->range = size;
+ }
+
+ if (kretprobe_blacklist_size) {
+ /* lookup the function address from its name */
+ for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
+ kprobe_lookup_name(kretprobe_blacklist[i].name,
+ kretprobe_blacklist[i].addr);
+ if (!kretprobe_blacklist[i].addr)
+ printk("kretprobe: lookup failed: %s\n",
+ kretprobe_blacklist[i].name);
+ }
+ }
+
+ smp_wmb();
+ kprobe_blacklist_initialized = true;
+
+out:
+ mutex_unlock(_mutex);
+}
+
#ifdef __ARCH_WANT_KPROBES_INSN_SLOT
/*
* kprobe->ainsn.insn points to the copy of the instruction to be
@@ -1331,6 +1388,9 @@ static int __kprobes in_kprobes_functions(unsigned long
addr)
if (addr >= (unsigned long)__kprobes_text_start &&
addr < (unsigned long)__kprobes_text_end)
return -EINVAL;
+
+ if (unlikely(!kprobe_blacklist_initialized))
+ init_kprobe_blacklist();
/*
* If there exists a kprobe_blacklist, verify and
* fail any probe registration in the prohibited area
@@ -1816,6 +1876,8 @@ int __kprobes register_kretprobe(struct kretprobe *rp)
void *addr;
if (kretprobe_blacklist_size) {
+ if (unlikely(!kprobe_blacklist_initialized))
+ init_kprobe_blacklist();
addr = kprobe_addr(>kp);
if (IS_ERR(addr))
return PTR_ERR(addr);
@@ -2065,11 +2127,6 @@ static struct notifier_block kprobe_module_nb = {
static int __init init_kprobes(void)
{
int i, err = 0;
- unsigned long offset = 0, size = 0;
- char *modname, namebuf[128];
- const char *symbol_name;
- void *addr;
- struct kprobe_blackpoint *kb;
/* FIXME allocate the probe table, currently defined statically */
/* initialize all list heads */
@@ -2079,39 +2136,6 @@ static int __init init_kprobes(void)
raw_spin_lock_init(&(kretprobe_table_locks[i].lock));
}
- /*
-* Lookup and populate the kprobe_blacklist.
-*
-* Unlike the kretprobe blacklist, we'll need to determine
-* the range of addresses that belong to the said functions,
-* since a kprobe need not necessarily be at the beginning
-* of a function.
-*/
- for (kb = kprobe_blacklist; kb->name != NULL; kb++) {
- kprobe_lookup_name(kb->name, addr);
- if (!addr)
- continue;
-
- kb->start_addr = (unsigned long)addr;
- symbol_name = kallsyms_look
[PATCH v3 3/4] kprobes: move x86-specific blacklist symbols to arch directory
From: Björn Davidsson
The common kprobes blacklist contains x86-specific symbols.
Looking for these in kallsyms takes unnecessary time during startup
on non-X86 platform. The symbols where moved to
arch/x86/kernel/kprobes/core.c.
Cc: Masami Hiramatsu
Cc: David S. Miller
Cc: linux-a...@vger.kernel.org
Reviewed-by: Radovan Lekanovic
Signed-off-by: Björn Davidsson
Signed-off-by: Oskar Andero
Acked-by: Masami Hiramatsu
---
arch/x86/kernel/kprobes/core.c | 7 +++
kernel/kprobes.c | 3 ---
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 7bfe318..41ce6c1 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -65,6 +65,13 @@ void jprobe_return_end(void);
DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
+const char * const arch_kprobes_blacksyms[] = {
+ "native_get_debugreg",
+ "irq_entries_start",
+ "common_interrupt",
+};
+const size_t arch_kprobes_blacksyms_size = ARRAY_SIZE(arch_kprobes_blacksyms);
+
#define stack_addr(regs) ((unsigned long *)kernel_stack_pointer(regs))
#define W(row, b0, b1, b2, b3, b4, b5, b6, b7, b8, b9, ba, bb, bc, bd, be, bf)\
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 2458ae1..b289384 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -95,9 +95,6 @@ static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long
hash)
*/
static const char * const common_kprobes_blacksyms[] = {
"preempt_schedule",
- "native_get_debugreg",
- "irq_entries_start",
- "common_interrupt",
"mcount", /* mcount can be called from everywhere */
};
static const size_t common_kprobes_blacksyms_size =
--
1.8.1.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 v3 2/4] kprobes: split blacklist into common and arch
Some blackpoints are only valid for specific architectures. To let each
architecture specify its own blackpoints the list has been split in two
lists: common and arch. The common list is kept in kernel/kprobes.c and
the arch list is kept in the arch/ directory.
Cc: Masami Hiramatsu
Cc: David S. Miller
Cc: linux-a...@vger.kernel.org
Signed-off-by: Oskar Andero
---
kernel/kprobes.c | 88 +---
1 file changed, 59 insertions(+), 29 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index c8c2281..2458ae1 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -68,7 +68,6 @@
#endif
static int kprobes_initialized;
-static bool kprobe_blacklist_initialized;
static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
@@ -94,31 +93,64 @@ static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned
long hash)
*
* For such cases, we now have a blacklist
*/
-static struct kprobe_blackpoint kprobe_blacklist[] = {
- {"preempt_schedule",},
- {"native_get_debugreg",},
- {"irq_entries_start",},
- {"common_interrupt",},
- {"mcount",},/* mcount can be called from everywhere */
- {NULL}/* Terminator */
+static const char * const common_kprobes_blacksyms[] = {
+ "preempt_schedule",
+ "native_get_debugreg",
+ "irq_entries_start",
+ "common_interrupt",
+ "mcount", /* mcount can be called from everywhere */
};
+static const size_t common_kprobes_blacksyms_size =
+ ARRAY_SIZE(common_kprobes_blacksyms);
+
+/*
+ * These weak symbols can be overridden from the arch/ directory for
+ * architecure specific blackpoints.
+ */
+const char * const __weak arch_kprobes_blacksyms[] = {};
+const size_t __weak arch_kprobes_blacksyms_size;
+
+static struct kprobe_blackpoint *kprobe_blacklist;
+static size_t kprobe_blacklist_size;
+
+static void init_kprobe_blacklist_entry(struct kprobe_blackpoint *kb,
+ const char * const name)
+{
+ const char *symbol_name;
+ char *modname, namebuf[128];
+ void *addr;
+ unsigned long offset = 0, size = 0;
+
+ kb->name = name;
+ kprobe_lookup_name(kb->name, addr);
+ if (!addr)
+ return;
+
+ kb->start_addr = (unsigned long)addr;
+ symbol_name = kallsyms_lookup(kb->start_addr,
+ , , , namebuf);
+ if (!symbol_name)
+ kb->range = 0;
+ else
+ kb->range = size;
+}
/* it can take some time ( > 100ms ) to initialise the
* blacklist so we delay this until we actually need it
*/
static void init_kprobe_blacklist(void)
{
- int i;
- unsigned long offset = 0, size = 0;
- char *modname, namebuf[128];
- const char *symbol_name;
- void *addr;
+ int i, j = 0;
struct kprobe_blackpoint *kb;
mutex_lock(_mutex);
- if (kprobe_blacklist_initialized)
+ if (kprobe_blacklist)
goto out;
+ kprobe_blacklist_size = common_kprobes_blacksyms_size +
+ arch_kprobes_blacksyms_size;
+ kb = kzalloc(sizeof(*kb) * kprobe_blacklist_size, GFP_KERNEL);
+
/*
* Lookup and populate the kprobe_blacklist.
*
@@ -127,18 +159,14 @@ static void init_kprobe_blacklist(void)
* since a kprobe need not necessarily be at the beginning
* of a function.
*/
- for (kb = kprobe_blacklist; kb->name != NULL; kb++) {
- kprobe_lookup_name(kb->name, addr);
- if (!addr)
- continue;
+ for (i = 0; i < common_kprobes_blacksyms_size; i++, j++) {
+ init_kprobe_blacklist_entry([j],
+ common_kprobes_blacksyms[i]);
+ }
- kb->start_addr = (unsigned long)addr;
- symbol_name = kallsyms_lookup(kb->start_addr,
- , , , namebuf);
- if (!symbol_name)
- kb->range = 0;
- else
- kb->range = size;
+ for (i = 0; i < arch_kprobes_blacksyms_size; i++, j++) {
+ init_kprobe_blacklist_entry([j],
+ arch_kprobes_blacksyms[i]);
}
if (kretprobe_blacklist_size) {
@@ -153,7 +181,7 @@ static void init_kprobe_blacklist(void)
}
smp_wmb();
- kprobe_blacklist_initialized = true;
+ kprobe_blacklist = kb;
out:
mutex_unlock(_mutex);
@@ -1384,18 +1412,20 @@ out:
static int __kprobes in_kprobes_functions(unsigned long addr)
{
struct kprobe_blackpoint *kb;
+ int i;
if (addr >=
[PATCH v3 0/4] kprobes: split blacklist into common and arch
Hi, This is version 3 of the patch-set for splitting arch and common kprobe blackpoints. Changes since last version are: - 1/4: Add write memory barrier at blacklist initialization. - 1/4: Change kprobe_blacklist_initialized type to boolean. - 2/4: Fix racing of kprobe_blacklist. - 2/4: Define arch_kprobes_blacksyms.* as __weak symbols. -Oskar Björn Davidsson (1): kprobes: move x86-specific blacklist symbols to arch directory Oskar Andero (2): kprobes: split blacklist into common and arch kprobes: replace printk with pr_-functions Toby Collett (1): kprobes: delay blacklist symbol lookup until we actually need it arch/x86/kernel/kprobes/core.c | 7 ++ kernel/kprobes.c | 159 +++-- 2 files changed, 112 insertions(+), 54 deletions(-) -- 1.8.1.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 v3 4/4] kprobes: replace printk with pr_-functions
Instead of using printk use pr_info/pr_err/pr_warn. This was
detected by the checkpatch.pl script.
Cc: Masami Hiramatsu
Cc: David S. Miller
Signed-off-by: Oskar Andero
Acked-by: Masami Hiramatsu
---
kernel/kprobes.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index b289384..08facfc 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -172,7 +172,7 @@ static void init_kprobe_blacklist(void)
kprobe_lookup_name(kretprobe_blacklist[i].name,
kretprobe_blacklist[i].addr);
if (!kretprobe_blacklist[i].addr)
- printk("kretprobe: lookup failed: %s\n",
+ pr_err("kretprobe: lookup failed: %s\n",
kretprobe_blacklist[i].name);
}
}
@@ -787,7 +787,7 @@ static void reuse_unused_kprobe(struct kprobe *ap)
*/
op = container_of(ap, struct optimized_kprobe, kp);
if (unlikely(list_empty(>list)))
- printk(KERN_WARNING "Warning: found a stray unused "
+ pr_warn("Warning: found a stray unused "
"aggrprobe@%p\n", ap->addr);
/* Enable the probe again */
ap->flags &= ~KPROBE_FLAG_DISABLED;
@@ -894,7 +894,7 @@ static void __kprobes optimize_all_kprobes(void)
if (!kprobe_disabled(p))
optimize_kprobe(p);
}
- printk(KERN_INFO "Kprobes globally optimized\n");
+ pr_info("Kprobes globally optimized\n");
}
/* This should be called with kprobe_mutex locked */
@@ -918,7 +918,7 @@ static void __kprobes unoptimize_all_kprobes(void)
}
/* Wait for unoptimizing completion */
wait_for_kprobe_optimizer();
- printk(KERN_INFO "Kprobes globally unoptimized\n");
+ pr_info("Kprobes globally unoptimized\n");
}
int sysctl_kprobes_optimization;
@@ -989,7 +989,7 @@ static void __kprobes __disarm_kprobe(struct kprobe *p,
bool reopt)
/* There should be no unused kprobes can be reused without optimization */
static void reuse_unused_kprobe(struct kprobe *ap)
{
- printk(KERN_ERR "Error: There should be no unused kprobe here.\n");
+ pr_err("Error: There should be no unused kprobe here.\n");
BUG_ON(kprobe_unused(ap));
}
@@ -2103,8 +2103,8 @@ EXPORT_SYMBOL_GPL(enable_kprobe);
void __kprobes dump_kprobe(struct kprobe *kp)
{
- printk(KERN_WARNING "Dumping kprobe:\n");
- printk(KERN_WARNING "Name: %s\nAddress: %p\nOffset: %x\n",
+ pr_warn("Dumping kprobe:\n");
+ pr_warn("Name: %s\nAddress: %p\nOffset: %x\n",
kp->symbol_name, kp->addr, kp->offset);
}
@@ -2300,7 +2300,7 @@ static void __kprobes arm_all_kprobes(void)
}
kprobes_all_disarmed = false;
- printk(KERN_INFO "Kprobes globally enabled\n");
+ pr_info("Kprobes globally enabled\n");
already_enabled:
mutex_unlock(_mutex);
@@ -2322,7 +2322,7 @@ static void __kprobes disarm_all_kprobes(void)
}
kprobes_all_disarmed = true;
- printk(KERN_INFO "Kprobes globally disabled\n");
+ pr_info("Kprobes globally disabled\n");
for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
head = _table[i];
--
1.8.1.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: Re: [PATCH v2 1/4] kprobes: delay blacklist symbol lookup until we actually need it
Thanks for your input guys!
On 04:16 Fri 05 Apr , Masami Hiramatsu wrote:
> (2013/04/05 9:56), Joonsoo Kim wrote:
> > Hello, Oskar.
> >
> > On Thu, Apr 04, 2013 at 02:51:26PM +0200, Oskar Andero wrote:
> >> From: Toby Collett
> >>
> >> The symbol lookup can take a long time and kprobes is
> >> initialised very early in boot, so delay symbol lookup
> >> until the blacklist is first used.
> >>
> >> Cc: Masami Hiramatsu
> >> Cc: David S. Miller
> >> Reviewed-by: Radovan Lekanovic
> >> Signed-off-by: Toby Collett
> >> Signed-off-by: Oskar Andero
> >> ---
> >> kernel/kprobes.c | 98
> >> ++--
> >> 1 file changed, 60 insertions(+), 38 deletions(-)
> >>
> >> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> >> index e35be53..0a270e5 100644
> >> --- a/kernel/kprobes.c
> >> +++ b/kernel/kprobes.c
> >> @@ -68,6 +68,7 @@
> >> #endif
> >>
> >> static int kprobes_initialized;
> >> +static int kprobe_blacklist_initialized;
> >> static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
> >> static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
> >>
> >> @@ -102,6 +103,60 @@ static struct kprobe_blackpoint kprobe_blacklist[] = {
> >>{NULL}/* Terminator */
> >> };
> >>
> >> +/* it can take some time ( > 100ms ) to initialise the
> >> + * blacklist so we delay this until we actually need it
> >> + */
> >> +static void init_kprobe_blacklist(void)
> >> +{
> >> + int i;
> >> + unsigned long offset = 0, size = 0;
> >> + char *modname, namebuf[128];
> >> + const char *symbol_name;
> >> + void *addr;
> >> + struct kprobe_blackpoint *kb;
> >> +
> >> + mutex_lock(_mutex);
> >> + if (kprobe_blacklist_initialized)
> >> + goto out;
> >> +
> >> + /*
> >> + * Lookup and populate the kprobe_blacklist.
> >> + *
> >> + * Unlike the kretprobe blacklist, we'll need to determine
> >> + * the range of addresses that belong to the said functions,
> >> + * since a kprobe need not necessarily be at the beginning
> >> + * of a function.
> >> + */
> >> + for (kb = kprobe_blacklist; kb->name != NULL; kb++) {
> >> + kprobe_lookup_name(kb->name, addr);
> >> + if (!addr)
> >> + continue;
> >> +
> >> + kb->start_addr = (unsigned long)addr;
> >> + symbol_name = kallsyms_lookup(kb->start_addr,
> >> + , , , namebuf);
> >> + if (!symbol_name)
> >> + kb->range = 0;
> >> + else
> >> + kb->range = size;
> >> + }
> >> +
> >> + if (kretprobe_blacklist_size) {
> >> + /* lookup the function address from its name */
> >> + for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
> >> + kprobe_lookup_name(kretprobe_blacklist[i].name,
> >> + kretprobe_blacklist[i].addr);
> >> + if (!kretprobe_blacklist[i].addr)
> >> + printk("kretprobe: lookup failed: %s\n",
> >> + kretprobe_blacklist[i].name);
> >> + }
> >> + }
> >> + kprobe_blacklist_initialized = 1;
> >
> > You need smp_wmb() before assigning 'kprobe_blacklist_initialized = 1'.
> > This guarantee that who see kprobe_blacklist_initialized = 1 will get
> > updated data of kprobe_blacklist.
>
> Right, to ensure blacklist is updated, memory barrier is required.
Agreed.
> > Please refer my previous patch once more :)
> >
> > And How about define kprobe_blacklist_initialized as boolean?
>
> Good idea :)
>
I'll fix it for v3.
-Oskar
>
--
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] input: don't call input_dev_release_keys() in resume
On 18:33 Thu 04 Apr , Dmitry Torokhov wrote: > Hi Oskar, > > On Thu, Mar 07, 2013 at 03:01:22PM +0100, oskar.and...@sonymobile.com wrote: > > From: Aleksej Makarov > > > > When waking up the platform by pressing a specific key, sending a > > release on that key makes it impossible to react on the event in > > user-space. > > > > No, we can not simply not release keys after resume from suspend, as > this leads to keys being stuck. Consider you are holding an 'I' key on > your external USB keyboard and close your laptop's lid. Then you release > the key and leave. Later you come back, open the lid waking the laptop > and observe endless stream of 'I' in your open terminal. You're absolutely right. But I guess you also see the case that the patch is trying to fix, right? To explain our use-case: We have a physical camera button on some devices. When the user long-presses the button, the system will wake up and jump directly in to the camera application. > Maybe we should release the keys during suspend time? I am not sure how > Android infrastructure will react to this though... That sounds like a good idea! Let me do some testing on that. Thanks! -Oskar -- 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] input: don't call input_dev_release_keys() in resume
On 18:33 Thu 04 Apr , Dmitry Torokhov wrote: Hi Oskar, On Thu, Mar 07, 2013 at 03:01:22PM +0100, oskar.and...@sonymobile.com wrote: From: Aleksej Makarov aleksej.maka...@sonymobile.com When waking up the platform by pressing a specific key, sending a release on that key makes it impossible to react on the event in user-space. No, we can not simply not release keys after resume from suspend, as this leads to keys being stuck. Consider you are holding an 'I' key on your external USB keyboard and close your laptop's lid. Then you release the key and leave. Later you come back, open the lid waking the laptop and observe endless stream of 'I' in your open terminal. You're absolutely right. But I guess you also see the case that the patch is trying to fix, right? To explain our use-case: We have a physical camera button on some devices. When the user long-presses the button, the system will wake up and jump directly in to the camera application. Maybe we should release the keys during suspend time? I am not sure how Android infrastructure will react to this though... That sounds like a good idea! Let me do some testing on that. Thanks! -Oskar -- 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: Re: [PATCH v2 1/4] kprobes: delay blacklist symbol lookup until we actually need it
Thanks for your input guys!
On 04:16 Fri 05 Apr , Masami Hiramatsu wrote:
(2013/04/05 9:56), Joonsoo Kim wrote:
Hello, Oskar.
On Thu, Apr 04, 2013 at 02:51:26PM +0200, Oskar Andero wrote:
From: Toby Collett toby.coll...@sonymobile.com
The symbol lookup can take a long time and kprobes is
initialised very early in boot, so delay symbol lookup
until the blacklist is first used.
Cc: Masami Hiramatsu masami.hiramatsu...@hitachi.com
Cc: David S. Miller da...@davemloft.net
Reviewed-by: Radovan Lekanovic radovan.lekano...@sonymobile.com
Signed-off-by: Toby Collett toby.coll...@sonymobile.com
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
---
kernel/kprobes.c | 98
++--
1 file changed, 60 insertions(+), 38 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index e35be53..0a270e5 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -68,6 +68,7 @@
#endif
static int kprobes_initialized;
+static int kprobe_blacklist_initialized;
static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
@@ -102,6 +103,60 @@ static struct kprobe_blackpoint kprobe_blacklist[] = {
{NULL}/* Terminator */
};
+/* it can take some time ( 100ms ) to initialise the
+ * blacklist so we delay this until we actually need it
+ */
+static void init_kprobe_blacklist(void)
+{
+ int i;
+ unsigned long offset = 0, size = 0;
+ char *modname, namebuf[128];
+ const char *symbol_name;
+ void *addr;
+ struct kprobe_blackpoint *kb;
+
+ mutex_lock(kprobe_mutex);
+ if (kprobe_blacklist_initialized)
+ goto out;
+
+ /*
+ * Lookup and populate the kprobe_blacklist.
+ *
+ * Unlike the kretprobe blacklist, we'll need to determine
+ * the range of addresses that belong to the said functions,
+ * since a kprobe need not necessarily be at the beginning
+ * of a function.
+ */
+ for (kb = kprobe_blacklist; kb-name != NULL; kb++) {
+ kprobe_lookup_name(kb-name, addr);
+ if (!addr)
+ continue;
+
+ kb-start_addr = (unsigned long)addr;
+ symbol_name = kallsyms_lookup(kb-start_addr,
+ size, offset, modname, namebuf);
+ if (!symbol_name)
+ kb-range = 0;
+ else
+ kb-range = size;
+ }
+
+ if (kretprobe_blacklist_size) {
+ /* lookup the function address from its name */
+ for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
+ kprobe_lookup_name(kretprobe_blacklist[i].name,
+ kretprobe_blacklist[i].addr);
+ if (!kretprobe_blacklist[i].addr)
+ printk(kretprobe: lookup failed: %s\n,
+ kretprobe_blacklist[i].name);
+ }
+ }
+ kprobe_blacklist_initialized = 1;
You need smp_wmb() before assigning 'kprobe_blacklist_initialized = 1'.
This guarantee that who see kprobe_blacklist_initialized = 1 will get
updated data of kprobe_blacklist.
Right, to ensure blacklist is updated, memory barrier is required.
Agreed.
Please refer my previous patch once more :)
And How about define kprobe_blacklist_initialized as boolean?
Good idea :)
I'll fix it for v3.
-Oskar
--
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 v3 4/4] kprobes: replace printk with pr_-functions
Instead of using printk use pr_info/pr_err/pr_warn. This was
detected by the checkpatch.pl script.
Cc: Masami Hiramatsu masami.hiramatsu...@hitachi.com
Cc: David S. Miller da...@davemloft.net
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
Acked-by: Masami Hiramatsu masami.hiramatsu...@hitachi.com
---
kernel/kprobes.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index b289384..08facfc 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -172,7 +172,7 @@ static void init_kprobe_blacklist(void)
kprobe_lookup_name(kretprobe_blacklist[i].name,
kretprobe_blacklist[i].addr);
if (!kretprobe_blacklist[i].addr)
- printk(kretprobe: lookup failed: %s\n,
+ pr_err(kretprobe: lookup failed: %s\n,
kretprobe_blacklist[i].name);
}
}
@@ -787,7 +787,7 @@ static void reuse_unused_kprobe(struct kprobe *ap)
*/
op = container_of(ap, struct optimized_kprobe, kp);
if (unlikely(list_empty(op-list)))
- printk(KERN_WARNING Warning: found a stray unused
+ pr_warn(Warning: found a stray unused
aggrprobe@%p\n, ap-addr);
/* Enable the probe again */
ap-flags = ~KPROBE_FLAG_DISABLED;
@@ -894,7 +894,7 @@ static void __kprobes optimize_all_kprobes(void)
if (!kprobe_disabled(p))
optimize_kprobe(p);
}
- printk(KERN_INFO Kprobes globally optimized\n);
+ pr_info(Kprobes globally optimized\n);
}
/* This should be called with kprobe_mutex locked */
@@ -918,7 +918,7 @@ static void __kprobes unoptimize_all_kprobes(void)
}
/* Wait for unoptimizing completion */
wait_for_kprobe_optimizer();
- printk(KERN_INFO Kprobes globally unoptimized\n);
+ pr_info(Kprobes globally unoptimized\n);
}
int sysctl_kprobes_optimization;
@@ -989,7 +989,7 @@ static void __kprobes __disarm_kprobe(struct kprobe *p,
bool reopt)
/* There should be no unused kprobes can be reused without optimization */
static void reuse_unused_kprobe(struct kprobe *ap)
{
- printk(KERN_ERR Error: There should be no unused kprobe here.\n);
+ pr_err(Error: There should be no unused kprobe here.\n);
BUG_ON(kprobe_unused(ap));
}
@@ -2103,8 +2103,8 @@ EXPORT_SYMBOL_GPL(enable_kprobe);
void __kprobes dump_kprobe(struct kprobe *kp)
{
- printk(KERN_WARNING Dumping kprobe:\n);
- printk(KERN_WARNING Name: %s\nAddress: %p\nOffset: %x\n,
+ pr_warn(Dumping kprobe:\n);
+ pr_warn(Name: %s\nAddress: %p\nOffset: %x\n,
kp-symbol_name, kp-addr, kp-offset);
}
@@ -2300,7 +2300,7 @@ static void __kprobes arm_all_kprobes(void)
}
kprobes_all_disarmed = false;
- printk(KERN_INFO Kprobes globally enabled\n);
+ pr_info(Kprobes globally enabled\n);
already_enabled:
mutex_unlock(kprobe_mutex);
@@ -2322,7 +2322,7 @@ static void __kprobes disarm_all_kprobes(void)
}
kprobes_all_disarmed = true;
- printk(KERN_INFO Kprobes globally disabled\n);
+ pr_info(Kprobes globally disabled\n);
for (i = 0; i KPROBE_TABLE_SIZE; i++) {
head = kprobe_table[i];
--
1.8.1.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 v3 0/4] kprobes: split blacklist into common and arch
Hi, This is version 3 of the patch-set for splitting arch and common kprobe blackpoints. Changes since last version are: - 1/4: Add write memory barrier at blacklist initialization. - 1/4: Change kprobe_blacklist_initialized type to boolean. - 2/4: Fix racing of kprobe_blacklist. - 2/4: Define arch_kprobes_blacksyms.* as __weak symbols. -Oskar Björn Davidsson (1): kprobes: move x86-specific blacklist symbols to arch directory Oskar Andero (2): kprobes: split blacklist into common and arch kprobes: replace printk with pr_-functions Toby Collett (1): kprobes: delay blacklist symbol lookup until we actually need it arch/x86/kernel/kprobes/core.c | 7 ++ kernel/kprobes.c | 159 +++-- 2 files changed, 112 insertions(+), 54 deletions(-) -- 1.8.1.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 v3 2/4] kprobes: split blacklist into common and arch
Some blackpoints are only valid for specific architectures. To let each
architecture specify its own blackpoints the list has been split in two
lists: common and arch. The common list is kept in kernel/kprobes.c and
the arch list is kept in the arch/ directory.
Cc: Masami Hiramatsu masami.hiramatsu...@hitachi.com
Cc: David S. Miller da...@davemloft.net
Cc: linux-a...@vger.kernel.org
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
---
kernel/kprobes.c | 88 +---
1 file changed, 59 insertions(+), 29 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index c8c2281..2458ae1 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -68,7 +68,6 @@
#endif
static int kprobes_initialized;
-static bool kprobe_blacklist_initialized;
static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
@@ -94,31 +93,64 @@ static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned
long hash)
*
* For such cases, we now have a blacklist
*/
-static struct kprobe_blackpoint kprobe_blacklist[] = {
- {preempt_schedule,},
- {native_get_debugreg,},
- {irq_entries_start,},
- {common_interrupt,},
- {mcount,},/* mcount can be called from everywhere */
- {NULL}/* Terminator */
+static const char * const common_kprobes_blacksyms[] = {
+ preempt_schedule,
+ native_get_debugreg,
+ irq_entries_start,
+ common_interrupt,
+ mcount, /* mcount can be called from everywhere */
};
+static const size_t common_kprobes_blacksyms_size =
+ ARRAY_SIZE(common_kprobes_blacksyms);
+
+/*
+ * These weak symbols can be overridden from the arch/ directory for
+ * architecure specific blackpoints.
+ */
+const char * const __weak arch_kprobes_blacksyms[] = {};
+const size_t __weak arch_kprobes_blacksyms_size;
+
+static struct kprobe_blackpoint *kprobe_blacklist;
+static size_t kprobe_blacklist_size;
+
+static void init_kprobe_blacklist_entry(struct kprobe_blackpoint *kb,
+ const char * const name)
+{
+ const char *symbol_name;
+ char *modname, namebuf[128];
+ void *addr;
+ unsigned long offset = 0, size = 0;
+
+ kb-name = name;
+ kprobe_lookup_name(kb-name, addr);
+ if (!addr)
+ return;
+
+ kb-start_addr = (unsigned long)addr;
+ symbol_name = kallsyms_lookup(kb-start_addr,
+ size, offset, modname, namebuf);
+ if (!symbol_name)
+ kb-range = 0;
+ else
+ kb-range = size;
+}
/* it can take some time ( 100ms ) to initialise the
* blacklist so we delay this until we actually need it
*/
static void init_kprobe_blacklist(void)
{
- int i;
- unsigned long offset = 0, size = 0;
- char *modname, namebuf[128];
- const char *symbol_name;
- void *addr;
+ int i, j = 0;
struct kprobe_blackpoint *kb;
mutex_lock(kprobe_mutex);
- if (kprobe_blacklist_initialized)
+ if (kprobe_blacklist)
goto out;
+ kprobe_blacklist_size = common_kprobes_blacksyms_size +
+ arch_kprobes_blacksyms_size;
+ kb = kzalloc(sizeof(*kb) * kprobe_blacklist_size, GFP_KERNEL);
+
/*
* Lookup and populate the kprobe_blacklist.
*
@@ -127,18 +159,14 @@ static void init_kprobe_blacklist(void)
* since a kprobe need not necessarily be at the beginning
* of a function.
*/
- for (kb = kprobe_blacklist; kb-name != NULL; kb++) {
- kprobe_lookup_name(kb-name, addr);
- if (!addr)
- continue;
+ for (i = 0; i common_kprobes_blacksyms_size; i++, j++) {
+ init_kprobe_blacklist_entry(kb[j],
+ common_kprobes_blacksyms[i]);
+ }
- kb-start_addr = (unsigned long)addr;
- symbol_name = kallsyms_lookup(kb-start_addr,
- size, offset, modname, namebuf);
- if (!symbol_name)
- kb-range = 0;
- else
- kb-range = size;
+ for (i = 0; i arch_kprobes_blacksyms_size; i++, j++) {
+ init_kprobe_blacklist_entry(kb[j],
+ arch_kprobes_blacksyms[i]);
}
if (kretprobe_blacklist_size) {
@@ -153,7 +181,7 @@ static void init_kprobe_blacklist(void)
}
smp_wmb();
- kprobe_blacklist_initialized = true;
+ kprobe_blacklist = kb;
out:
mutex_unlock(kprobe_mutex);
@@ -1384,18 +1412,20 @@ out:
static int __kprobes in_kprobes_functions(unsigned long addr)
{
struct kprobe_blackpoint *kb;
+ int i;
if (addr = (unsigned long)__kprobes_text_start
addr (unsigned long
[PATCH v3 3/4] kprobes: move x86-specific blacklist symbols to arch directory
From: Björn Davidsson bjorn.davids...@sonymobile.com
The common kprobes blacklist contains x86-specific symbols.
Looking for these in kallsyms takes unnecessary time during startup
on non-X86 platform. The symbols where moved to
arch/x86/kernel/kprobes/core.c.
Cc: Masami Hiramatsu masami.hiramatsu...@hitachi.com
Cc: David S. Miller da...@davemloft.net
Cc: linux-a...@vger.kernel.org
Reviewed-by: Radovan Lekanovic radovan.lekano...@sonymobile.com
Signed-off-by: Björn Davidsson bjorn.davids...@sonymobile.com
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
Acked-by: Masami Hiramatsu masami.hiramatsu...@hitachi.com
---
arch/x86/kernel/kprobes/core.c | 7 +++
kernel/kprobes.c | 3 ---
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 7bfe318..41ce6c1 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -65,6 +65,13 @@ void jprobe_return_end(void);
DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
+const char * const arch_kprobes_blacksyms[] = {
+ native_get_debugreg,
+ irq_entries_start,
+ common_interrupt,
+};
+const size_t arch_kprobes_blacksyms_size = ARRAY_SIZE(arch_kprobes_blacksyms);
+
#define stack_addr(regs) ((unsigned long *)kernel_stack_pointer(regs))
#define W(row, b0, b1, b2, b3, b4, b5, b6, b7, b8, b9, ba, bb, bc, bd, be, bf)\
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 2458ae1..b289384 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -95,9 +95,6 @@ static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long
hash)
*/
static const char * const common_kprobes_blacksyms[] = {
preempt_schedule,
- native_get_debugreg,
- irq_entries_start,
- common_interrupt,
mcount, /* mcount can be called from everywhere */
};
static const size_t common_kprobes_blacksyms_size =
--
1.8.1.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 v3 1/4] kprobes: delay blacklist symbol lookup until we actually need it
From: Toby Collett toby.coll...@sonymobile.com
The symbol lookup can take a long time and kprobes is
initialised very early in boot, so delay symbol lookup
until the blacklist is first used.
Cc: Masami Hiramatsu masami.hiramatsu...@hitachi.com
Cc: David S. Miller da...@davemloft.net
Reviewed-by: Radovan Lekanovic radovan.lekano...@sonymobile.com
Signed-off-by: Toby Collett toby.coll...@sonymobile.com
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
---
kernel/kprobes.c | 100 ++-
1 file changed, 62 insertions(+), 38 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index e35be53..c8c2281 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -68,6 +68,7 @@
#endif
static int kprobes_initialized;
+static bool kprobe_blacklist_initialized;
static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
@@ -102,6 +103,62 @@ static struct kprobe_blackpoint kprobe_blacklist[] = {
{NULL}/* Terminator */
};
+/* it can take some time ( 100ms ) to initialise the
+ * blacklist so we delay this until we actually need it
+ */
+static void init_kprobe_blacklist(void)
+{
+ int i;
+ unsigned long offset = 0, size = 0;
+ char *modname, namebuf[128];
+ const char *symbol_name;
+ void *addr;
+ struct kprobe_blackpoint *kb;
+
+ mutex_lock(kprobe_mutex);
+ if (kprobe_blacklist_initialized)
+ goto out;
+
+ /*
+* Lookup and populate the kprobe_blacklist.
+*
+* Unlike the kretprobe blacklist, we'll need to determine
+* the range of addresses that belong to the said functions,
+* since a kprobe need not necessarily be at the beginning
+* of a function.
+*/
+ for (kb = kprobe_blacklist; kb-name != NULL; kb++) {
+ kprobe_lookup_name(kb-name, addr);
+ if (!addr)
+ continue;
+
+ kb-start_addr = (unsigned long)addr;
+ symbol_name = kallsyms_lookup(kb-start_addr,
+ size, offset, modname, namebuf);
+ if (!symbol_name)
+ kb-range = 0;
+ else
+ kb-range = size;
+ }
+
+ if (kretprobe_blacklist_size) {
+ /* lookup the function address from its name */
+ for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
+ kprobe_lookup_name(kretprobe_blacklist[i].name,
+ kretprobe_blacklist[i].addr);
+ if (!kretprobe_blacklist[i].addr)
+ printk(kretprobe: lookup failed: %s\n,
+ kretprobe_blacklist[i].name);
+ }
+ }
+
+ smp_wmb();
+ kprobe_blacklist_initialized = true;
+
+out:
+ mutex_unlock(kprobe_mutex);
+}
+
#ifdef __ARCH_WANT_KPROBES_INSN_SLOT
/*
* kprobe-ainsn.insn points to the copy of the instruction to be
@@ -1331,6 +1388,9 @@ static int __kprobes in_kprobes_functions(unsigned long
addr)
if (addr = (unsigned long)__kprobes_text_start
addr (unsigned long)__kprobes_text_end)
return -EINVAL;
+
+ if (unlikely(!kprobe_blacklist_initialized))
+ init_kprobe_blacklist();
/*
* If there exists a kprobe_blacklist, verify and
* fail any probe registration in the prohibited area
@@ -1816,6 +1876,8 @@ int __kprobes register_kretprobe(struct kretprobe *rp)
void *addr;
if (kretprobe_blacklist_size) {
+ if (unlikely(!kprobe_blacklist_initialized))
+ init_kprobe_blacklist();
addr = kprobe_addr(rp-kp);
if (IS_ERR(addr))
return PTR_ERR(addr);
@@ -2065,11 +2127,6 @@ static struct notifier_block kprobe_module_nb = {
static int __init init_kprobes(void)
{
int i, err = 0;
- unsigned long offset = 0, size = 0;
- char *modname, namebuf[128];
- const char *symbol_name;
- void *addr;
- struct kprobe_blackpoint *kb;
/* FIXME allocate the probe table, currently defined statically */
/* initialize all list heads */
@@ -2079,39 +2136,6 @@ static int __init init_kprobes(void)
raw_spin_lock_init((kretprobe_table_locks[i].lock));
}
- /*
-* Lookup and populate the kprobe_blacklist.
-*
-* Unlike the kretprobe blacklist, we'll need to determine
-* the range of addresses that belong to the said functions,
-* since a kprobe need not necessarily be at the beginning
-* of a function.
-*/
- for (kb = kprobe_blacklist; kb-name != NULL; kb++) {
- kprobe_lookup_name(kb-name, addr);
- if (!addr)
- continue
Re: [PATCH] input: don't call input_dev_release_keys() in resume
On 15:01 Thu 07 Mar , oskar.and...@sonymobile.com wrote:
> From: Aleksej Makarov
>
> When waking up the platform by pressing a specific key, sending a
> release on that key makes it impossible to react on the event in
> user-space.
>
> Cc: Dmitry Torokhov
> Reviewed-by: Radovan Lekanovic
> Signed-off-by: Aleksej Makarov
> Signed-off-by: Oskar Andero
> ---
> drivers/input/input.c |5 -
> 1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index c044699..61ce19f 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -1690,7 +1690,10 @@ static int input_dev_resume(struct device *dev)
> {
> struct input_dev *input_dev = to_input_dev(dev);
>
> - input_reset_device(input_dev);
> + mutex_lock(_dev->mutex);
> + if (input_dev->users)
> + input_dev_toggle(input_dev, true);
> + mutex_unlock(_dev->mutex);
>
> return 0;
> }
> --
> 1.7.8.6
>
Ping. Any input on the patch above?
-Oskar
--
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 v2 2/4] kprobes: split blacklist into common and arch
Some blackpoints are only valid for specific architectures. To let each
architecture specify its own blackpoints the list has been split in two
lists: common and arch. The common list is kept in kernel/kprobes.c and
the arch list is kept in the arch/ directory.
Cc: Masami Hiramatsu
Cc: David S. Miller
Cc: linux-a...@vger.kernel.org
Signed-off-by: Oskar Andero
---
arch/arc/kernel/kprobes.c | 3 ++
arch/arm/kernel/kprobes.c | 2 +
arch/avr32/kernel/kprobes.c| 3 ++
arch/ia64/kernel/kprobes.c | 3 ++
arch/mips/kernel/kprobes.c | 3 ++
arch/mn10300/kernel/kprobes.c | 2 +
arch/powerpc/kernel/kprobes.c | 3 ++
arch/s390/kernel/kprobes.c | 3 ++
arch/sh/kernel/kprobes.c | 3 ++
arch/sparc/kernel/kprobes.c| 3 ++
arch/x86/kernel/kprobes/core.c | 3 ++
kernel/kprobes.c | 85 +++---
12 files changed, 86 insertions(+), 30 deletions(-)
diff --git a/arch/arc/kernel/kprobes.c b/arch/arc/kernel/kprobes.c
index 3bfeacb..894eee6 100644
--- a/arch/arc/kernel/kprobes.c
+++ b/arch/arc/kernel/kprobes.c
@@ -24,6 +24,9 @@
DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
+const char * const arch_kprobes_blacksyms[] = {};
+const size_t arch_kprobes_blacksyms_size = ARRAY_SIZE(arch_kprobes_blacksyms);
+
int __kprobes arch_prepare_kprobe(struct kprobe *p)
{
/* Attempt to probe at unaligned address */
diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/kernel/kprobes.c
index 170e9f3..772d9ec 100644
--- a/arch/arm/kernel/kprobes.c
+++ b/arch/arm/kernel/kprobes.c
@@ -46,6 +46,8 @@
DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
+const char * const arch_kprobes_blacksyms[] = {};
+const size_t arch_kprobes_blacksyms_size = ARRAY_SIZE(arch_kprobes_blacksyms);
int __kprobes arch_prepare_kprobe(struct kprobe *p)
{
diff --git a/arch/avr32/kernel/kprobes.c b/arch/avr32/kernel/kprobes.c
index f820e9f..3b02c1e 100644
--- a/arch/avr32/kernel/kprobes.c
+++ b/arch/avr32/kernel/kprobes.c
@@ -24,6 +24,9 @@ static struct pt_regs jprobe_saved_regs;
struct kretprobe_blackpoint kretprobe_blacklist[] = {{NULL, NULL}};
+const char * const arch_kprobes_blacksyms[] = {};
+const size_t arch_kprobes_blacksyms_size = ARRAY_SIZE(arch_kprobes_blacksyms);
+
int __kprobes arch_prepare_kprobe(struct kprobe *p)
{
int ret = 0;
diff --git a/arch/ia64/kernel/kprobes.c b/arch/ia64/kernel/kprobes.c
index f8280a7..239f2fd 100644
--- a/arch/ia64/kernel/kprobes.c
+++ b/arch/ia64/kernel/kprobes.c
@@ -42,6 +42,9 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
struct kretprobe_blackpoint kretprobe_blacklist[] = {{NULL, NULL}};
+const char * const arch_kprobes_blacksyms[] = {};
+const size_t arch_kprobes_blacksyms_size = ARRAY_SIZE(arch_kprobes_blacksyms);
+
enum instruction_type {A, I, M, F, B, L, X, u};
static enum instruction_type bundle_encoding[32][3] = {
{ M, I, I }, /* 00 */
diff --git a/arch/mips/kernel/kprobes.c b/arch/mips/kernel/kprobes.c
index 12bc4eb..de6a1aa 100644
--- a/arch/mips/kernel/kprobes.c
+++ b/arch/mips/kernel/kprobes.c
@@ -53,6 +53,9 @@ static const union mips_instruction breakpoint2_insn = {
DEFINE_PER_CPU(struct kprobe *, current_kprobe);
DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
+const char * const arch_kprobes_blacksyms[] = {};
+const size_t arch_kprobes_blacksyms_size = ARRAY_SIZE(arch_kprobes_blacksyms);
+
static int __kprobes insn_has_delayslot(union mips_instruction insn)
{
switch (insn.i_format.opcode) {
diff --git a/arch/mn10300/kernel/kprobes.c b/arch/mn10300/kernel/kprobes.c
index 0311a7f..ed57094 100644
--- a/arch/mn10300/kernel/kprobes.c
+++ b/arch/mn10300/kernel/kprobes.c
@@ -41,6 +41,8 @@ static unsigned long cur_kprobe_bp_addr;
DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
+const char * const arch_kprobes_blacksyms[] = {};
+const size_t arch_kprobes_blacksyms_size = ARRAY_SIZE(arch_kprobes_blacksyms);
/* singlestep flag bits */
#define SINGLESTEP_BRANCH 1
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 11f5b03..b18ba45 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -47,6 +47,9 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
struct kretprobe_blackpoint kretprobe_blacklist[] = {{NULL, NULL}};
+const char * const arch_kprobes_blacksyms[] = {};
+const size_t arch_kprobes_blacksyms_size = ARRAY_SIZE(arch_kprobes_blacksyms);
+
int __kprobes arch_prepare_kprobe(struct kprobe *p)
{
int ret = 0;
diff --git a/arch/s390/kernel/kprobes.c b/arch/s390/kernel/kprobes.c
index 3388b2b..2077bb0 100644
--- a/arch/s390/kernel/kprobes.c
+++ b/arch/s390/kernel/kprobes.c
@@ -37,6 +37,9 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
struct kretprobe_blackpoint kretprobe_blacklist[] = { };
+const char * const
[PATCH v2 3/4] kprobes: move x86-specific blacklist symbols to arch directory
From: Björn Davidsson
The common kprobes blacklist contains x86-specific symbols.
Looking for these in kallsyms takes unnecessary time during startup
on non-X86 platform. The symbols where moved to
arch/x86/kernel/kprobes/core.c.
Cc: Masami Hiramatsu
Cc: David S. Miller
Cc: linux-a...@vger.kernel.org
Reviewed-by: Radovan Lekanovic
Signed-off-by: Björn Davidsson
Signed-off-by: Oskar Andero
---
arch/x86/kernel/kprobes/core.c | 6 +-
kernel/kprobes.c | 3 ---
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 4aa71a5..41ce6c1 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -65,7 +65,11 @@ void jprobe_return_end(void);
DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
-const char * const arch_kprobes_blacksyms[] = {};
+const char * const arch_kprobes_blacksyms[] = {
+ "native_get_debugreg",
+ "irq_entries_start",
+ "common_interrupt",
+};
const size_t arch_kprobes_blacksyms_size = ARRAY_SIZE(arch_kprobes_blacksyms);
#define stack_addr(regs) ((unsigned long *)kernel_stack_pointer(regs))
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 7654278..58c9f4e 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -95,9 +95,6 @@ static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long
hash)
*/
static const char * const common_kprobes_blacksyms[] = {
"preempt_schedule",
- "native_get_debugreg",
- "irq_entries_start",
- "common_interrupt",
"mcount", /* mcount can be called from everywhere */
};
static const size_t common_kprobes_blacksyms_size =
--
1.8.1.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 v2 0/4] kprobes: split blacklist into common and arch
Hi, This is version 2 of the patch-set for splitting arch and common kprobe blackpoints. Changes since last version are: - Don't use double pointer for blacklist. - Add mutex around blacklist initialization code. - Use unlikely in if-statement around init_kprobe_blacklist() calls. I have also included linux-arch in Cc for this post. -Oskar Björn Davidsson (1): kprobes: move x86-specific blacklist symbols to arch directory Oskar Andero (2): kprobes: split blacklist into common and arch kprobes: replace printk with pr_-functions Toby Collett (1): kprobes: delay blacklist symbol lookup until we actually need it arch/arc/kernel/kprobes.c | 3 + arch/arm/kernel/kprobes.c | 2 + arch/avr32/kernel/kprobes.c| 3 + arch/ia64/kernel/kprobes.c | 3 + arch/mips/kernel/kprobes.c | 3 + arch/mn10300/kernel/kprobes.c | 2 + arch/powerpc/kernel/kprobes.c | 3 + arch/s390/kernel/kprobes.c | 3 + arch/sh/kernel/kprobes.c | 3 + arch/sparc/kernel/kprobes.c| 3 + arch/x86/kernel/kprobes/core.c | 7 ++ kernel/kprobes.c | 152 ++--- 12 files changed, 133 insertions(+), 54 deletions(-) -- 1.8.1.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 v2 1/4] kprobes: delay blacklist symbol lookup until we actually need it
From: Toby Collett
The symbol lookup can take a long time and kprobes is
initialised very early in boot, so delay symbol lookup
until the blacklist is first used.
Cc: Masami Hiramatsu
Cc: David S. Miller
Reviewed-by: Radovan Lekanovic
Signed-off-by: Toby Collett
Signed-off-by: Oskar Andero
---
kernel/kprobes.c | 98 ++--
1 file changed, 60 insertions(+), 38 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index e35be53..0a270e5 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -68,6 +68,7 @@
#endif
static int kprobes_initialized;
+static int kprobe_blacklist_initialized;
static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
@@ -102,6 +103,60 @@ static struct kprobe_blackpoint kprobe_blacklist[] = {
{NULL}/* Terminator */
};
+/* it can take some time ( > 100ms ) to initialise the
+ * blacklist so we delay this until we actually need it
+ */
+static void init_kprobe_blacklist(void)
+{
+ int i;
+ unsigned long offset = 0, size = 0;
+ char *modname, namebuf[128];
+ const char *symbol_name;
+ void *addr;
+ struct kprobe_blackpoint *kb;
+
+ mutex_lock(_mutex);
+ if (kprobe_blacklist_initialized)
+ goto out;
+
+ /*
+* Lookup and populate the kprobe_blacklist.
+*
+* Unlike the kretprobe blacklist, we'll need to determine
+* the range of addresses that belong to the said functions,
+* since a kprobe need not necessarily be at the beginning
+* of a function.
+*/
+ for (kb = kprobe_blacklist; kb->name != NULL; kb++) {
+ kprobe_lookup_name(kb->name, addr);
+ if (!addr)
+ continue;
+
+ kb->start_addr = (unsigned long)addr;
+ symbol_name = kallsyms_lookup(kb->start_addr,
+ , , , namebuf);
+ if (!symbol_name)
+ kb->range = 0;
+ else
+ kb->range = size;
+ }
+
+ if (kretprobe_blacklist_size) {
+ /* lookup the function address from its name */
+ for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
+ kprobe_lookup_name(kretprobe_blacklist[i].name,
+ kretprobe_blacklist[i].addr);
+ if (!kretprobe_blacklist[i].addr)
+ printk("kretprobe: lookup failed: %s\n",
+ kretprobe_blacklist[i].name);
+ }
+ }
+ kprobe_blacklist_initialized = 1;
+
+out:
+ mutex_unlock(_mutex);
+}
+
#ifdef __ARCH_WANT_KPROBES_INSN_SLOT
/*
* kprobe->ainsn.insn points to the copy of the instruction to be
@@ -1331,6 +1386,9 @@ static int __kprobes in_kprobes_functions(unsigned long
addr)
if (addr >= (unsigned long)__kprobes_text_start &&
addr < (unsigned long)__kprobes_text_end)
return -EINVAL;
+
+ if (unlikely(!kprobe_blacklist_initialized))
+ init_kprobe_blacklist();
/*
* If there exists a kprobe_blacklist, verify and
* fail any probe registration in the prohibited area
@@ -1816,6 +1874,8 @@ int __kprobes register_kretprobe(struct kretprobe *rp)
void *addr;
if (kretprobe_blacklist_size) {
+ if (unlikely(!kprobe_blacklist_initialized))
+ init_kprobe_blacklist();
addr = kprobe_addr(>kp);
if (IS_ERR(addr))
return PTR_ERR(addr);
@@ -2065,11 +2125,6 @@ static struct notifier_block kprobe_module_nb = {
static int __init init_kprobes(void)
{
int i, err = 0;
- unsigned long offset = 0, size = 0;
- char *modname, namebuf[128];
- const char *symbol_name;
- void *addr;
- struct kprobe_blackpoint *kb;
/* FIXME allocate the probe table, currently defined statically */
/* initialize all list heads */
@@ -2079,39 +2134,6 @@ static int __init init_kprobes(void)
raw_spin_lock_init(&(kretprobe_table_locks[i].lock));
}
- /*
-* Lookup and populate the kprobe_blacklist.
-*
-* Unlike the kretprobe blacklist, we'll need to determine
-* the range of addresses that belong to the said functions,
-* since a kprobe need not necessarily be at the beginning
-* of a function.
-*/
- for (kb = kprobe_blacklist; kb->name != NULL; kb++) {
- kprobe_lookup_name(kb->name, addr);
- if (!addr)
- continue;
-
- kb->start_addr = (unsigned long)addr;
- symbol_name = kallsyms_lookup(kb->start_addr,
-
[PATCH v2 4/4] kprobes: replace printk with pr_-functions
Instead of using printk use pr_info/pr_err/pr_warn. This was
detected by the checkpatch.pl script.
Cc: Masami Hiramatsu
Cc: David S. Miller
Signed-off-by: Oskar Andero
Acked-by: Masami Hiramatsu
---
kernel/kprobes.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 58c9f4e..d61cbad 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -168,7 +168,7 @@ static void init_kprobe_blacklist(void)
kprobe_lookup_name(kretprobe_blacklist[i].name,
kretprobe_blacklist[i].addr);
if (!kretprobe_blacklist[i].addr)
- printk("kretprobe: lookup failed: %s\n",
+ pr_err("kretprobe: lookup failed: %s\n",
kretprobe_blacklist[i].name);
}
}
@@ -780,7 +780,7 @@ static void reuse_unused_kprobe(struct kprobe *ap)
*/
op = container_of(ap, struct optimized_kprobe, kp);
if (unlikely(list_empty(>list)))
- printk(KERN_WARNING "Warning: found a stray unused "
+ pr_warn("Warning: found a stray unused "
"aggrprobe@%p\n", ap->addr);
/* Enable the probe again */
ap->flags &= ~KPROBE_FLAG_DISABLED;
@@ -887,7 +887,7 @@ static void __kprobes optimize_all_kprobes(void)
if (!kprobe_disabled(p))
optimize_kprobe(p);
}
- printk(KERN_INFO "Kprobes globally optimized\n");
+ pr_info("Kprobes globally optimized\n");
}
/* This should be called with kprobe_mutex locked */
@@ -911,7 +911,7 @@ static void __kprobes unoptimize_all_kprobes(void)
}
/* Wait for unoptimizing completion */
wait_for_kprobe_optimizer();
- printk(KERN_INFO "Kprobes globally unoptimized\n");
+ pr_info("Kprobes globally unoptimized\n");
}
int sysctl_kprobes_optimization;
@@ -982,7 +982,7 @@ static void __kprobes __disarm_kprobe(struct kprobe *p,
bool reopt)
/* There should be no unused kprobes can be reused without optimization */
static void reuse_unused_kprobe(struct kprobe *ap)
{
- printk(KERN_ERR "Error: There should be no unused kprobe here.\n");
+ pr_err("Error: There should be no unused kprobe here.\n");
BUG_ON(kprobe_unused(ap));
}
@@ -2096,8 +2096,8 @@ EXPORT_SYMBOL_GPL(enable_kprobe);
void __kprobes dump_kprobe(struct kprobe *kp)
{
- printk(KERN_WARNING "Dumping kprobe:\n");
- printk(KERN_WARNING "Name: %s\nAddress: %p\nOffset: %x\n",
+ pr_warn("Dumping kprobe:\n");
+ pr_warn("Name: %s\nAddress: %p\nOffset: %x\n",
kp->symbol_name, kp->addr, kp->offset);
}
@@ -2293,7 +2293,7 @@ static void __kprobes arm_all_kprobes(void)
}
kprobes_all_disarmed = false;
- printk(KERN_INFO "Kprobes globally enabled\n");
+ pr_info("Kprobes globally enabled\n");
already_enabled:
mutex_unlock(_mutex);
@@ -2315,7 +2315,7 @@ static void __kprobes disarm_all_kprobes(void)
}
kprobes_all_disarmed = true;
- printk(KERN_INFO "Kprobes globally disabled\n");
+ pr_info("Kprobes globally disabled\n");
for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
head = _table[i];
--
1.8.1.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 2/4] kprobes: split blacklist into common and arch
On 08:17 Thu 04 Apr , Masami Hiramatsu wrote: > (2013/04/03 17:28), oskar.and...@sonymobile.com wrote: > > Some blackpoints are only valid for specific architectures. To let each > > architecture specify its own blackpoints the list has been split in two > > lists: common and arch. The common list is kept in kernel/kprobes.c and > > the arch list is kept in the arch/ directory. > > Hmm, I think you'd better merge this with patch 3/4. IMHO I think it's more logical to keep them separated instead of squashing a generic patch with an architecture specific patch. Also, there is the matter of authorship and credits. Of course, it's your call, so let me know if this is ok with you. > [...] > > +static const size_t common_kprobes_blacksyms_size = > > + ARRAY_SIZE(common_kprobes_blacksyms); > > + > > +extern const char * const arch_kprobes_blacksyms[]; > > +extern const size_t arch_kprobes_blacksyms_size; > > + > > +static struct kprobe_blackpoint **kprobe_blacklist; > > +static size_t kprobe_blacklist_size; > > Since the blacklist is allocated once and never be updated, > we just need an array of struct kprobe_blackpoint, no need > to allocate each entry. Right. I'll change that. Thanks! -Oskar -- 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 0/4] kprobes: split blacklist into common and arch
On 13:32 Thu 04 Apr , Vineet Gupta wrote: > Hi Oskar, > > On 04/03/2013 01:58 PM, oskar.and...@sonymobile.com wrote: > > Hi, > > > > This is a slight rework of the following patches which I posted earlier: > > [PATCH] Kprobes blacklist: Conditionally add x86-specific symbols > > [PATCH] delay blacklist symbol lookup until we actually need it > > > > This serie includes a patch that moves the architecture dependent black > > points > > to the arch/ directory and the x86 patch was modified to follow that patch. > > > > -Oskar > > > > Björn Davidsson (1): > > kprobes: move x86-specific blacklist symbols to arch directory > > > > Oskar Andero (2): > > kprobes: split blacklist into common and arch > > kprobes: replace printk with pr_-functions > > > > Toby Collett (1): > > kprobes: delay blacklist symbol lookup until we actually need it > > > > arch/arc/kernel/kprobes.c | 3 + > > arch/arm/kernel/kprobes.c | 2 + > > arch/avr32/kernel/kprobes.c| 3 + > > arch/ia64/kernel/kprobes.c | 3 + > > arch/mips/kernel/kprobes.c | 3 + > > arch/mn10300/kernel/kprobes.c | 2 + > > arch/powerpc/kernel/kprobes.c | 3 + > > arch/s390/kernel/kprobes.c | 3 + > > arch/sh/kernel/kprobes.c | 3 + > > arch/sparc/kernel/kprobes.c| 3 + > > arch/x86/kernel/kprobes/core.c | 7 ++ > > kernel/kprobes.c | 149 > > ++--- > > 12 files changed, 130 insertions(+), 54 deletions(-) > > > > For any arch changes, however trivial they might be, you need to CC the > respective > maintainers, or atleast post the patches on linux-arch so we know what's > going on ! Thanks Vineet! I will make sure to add linux-arch on CC for version 2. -Oskar -- 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 1/4] kprobes: delay blacklist symbol lookup until we actually need it
On 08:44 Thu 04 Apr , Masami Hiramatsu wrote:
> (2013/04/03 17:28), oskar.and...@sonymobile.com wrote:
> >
> > +/* it can take some time ( > 100ms ) to initialise the
> > + * blacklist so we delay this until we actually need it
> > + */
> > +static void init_kprobe_blacklist(void)
> > +{
> > + int i;
> > + unsigned long offset = 0, size = 0;
> > + char *modname, namebuf[128];
> > + const char *symbol_name;
> > + void *addr;
> > + struct kprobe_blackpoint *kb;
> > +
> > + /*
> > +* Lookup and populate the kprobe_blacklist.
> > +*
> > +* Unlike the kretprobe blacklist, we'll need to determine
> > +* the range of addresses that belong to the said functions,
> > +* since a kprobe need not necessarily be at the beginning
> > +* of a function.
> > +*/
> > + for (kb = kprobe_blacklist; kb->name != NULL; kb++) {
> > + kprobe_lookup_name(kb->name, addr);
> > + if (!addr)
> > + continue;
> > +
> > + kb->start_addr = (unsigned long)addr;
> > + symbol_name = kallsyms_lookup(kb->start_addr,
> > + , , , namebuf);
> > + if (!symbol_name)
> > + kb->range = 0;
> > + else
> > + kb->range = size;
> > + }
> > +
> > + if (kretprobe_blacklist_size) {
> > + /* lookup the function address from its name */
> > + for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
> > + kprobe_lookup_name(kretprobe_blacklist[i].name,
> > + kretprobe_blacklist[i].addr);
> > + if (!kretprobe_blacklist[i].addr)
> > + printk("kretprobe: lookup failed: %s\n",
> > + kretprobe_blacklist[i].name);
> > + }
> > + }
> > + kprobe_blacklist_initialized = 1;
> > +}
> > +
> > #ifdef __ARCH_WANT_KPROBES_INSN_SLOT
> > /*
> > * kprobe->ainsn.insn points to the copy of the instruction to be
> > @@ -1331,6 +1379,9 @@ static int __kprobes in_kprobes_functions(unsigned
> > long addr)
> > if (addr >= (unsigned long)__kprobes_text_start &&
> > addr < (unsigned long)__kprobes_text_end)
> > return -EINVAL;
> > +
> > + if (!kprobe_blacklist_initialized)
> > + init_kprobe_blacklist();
> > /*
> > * If there exists a kprobe_blacklist, verify and
> > * fail any probe registration in the prohibited area
> > @@ -1816,6 +1867,8 @@ int __kprobes register_kretprobe(struct kretprobe *rp)
> > void *addr;
> >
> > if (kretprobe_blacklist_size) {
> > + if (!kprobe_blacklist_initialized)
> > + init_kprobe_blacklist();
>
> Joonsoo reminds me that these calling points are not protected by
> kprobe_mutex,
> thus we have to do something for avoiding concurrent initialization.
>
> Perhaps, the easiest way is to protect init_kprobe_blacklist() by kprobe_mutex
> and check kprobe_blacklist_initialized again in the top of that.
Yes, you are right. I had a second look at Joonsoo's patch and I will
add a similar mutex for v2.
-Oskar
--
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 1/4] kprobes: delay blacklist symbol lookup until we actually need it
On 08:44 Thu 04 Apr , Masami Hiramatsu wrote:
(2013/04/03 17:28), oskar.and...@sonymobile.com wrote:
+/* it can take some time ( 100ms ) to initialise the
+ * blacklist so we delay this until we actually need it
+ */
+static void init_kprobe_blacklist(void)
+{
+ int i;
+ unsigned long offset = 0, size = 0;
+ char *modname, namebuf[128];
+ const char *symbol_name;
+ void *addr;
+ struct kprobe_blackpoint *kb;
+
+ /*
+* Lookup and populate the kprobe_blacklist.
+*
+* Unlike the kretprobe blacklist, we'll need to determine
+* the range of addresses that belong to the said functions,
+* since a kprobe need not necessarily be at the beginning
+* of a function.
+*/
+ for (kb = kprobe_blacklist; kb-name != NULL; kb++) {
+ kprobe_lookup_name(kb-name, addr);
+ if (!addr)
+ continue;
+
+ kb-start_addr = (unsigned long)addr;
+ symbol_name = kallsyms_lookup(kb-start_addr,
+ size, offset, modname, namebuf);
+ if (!symbol_name)
+ kb-range = 0;
+ else
+ kb-range = size;
+ }
+
+ if (kretprobe_blacklist_size) {
+ /* lookup the function address from its name */
+ for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
+ kprobe_lookup_name(kretprobe_blacklist[i].name,
+ kretprobe_blacklist[i].addr);
+ if (!kretprobe_blacklist[i].addr)
+ printk(kretprobe: lookup failed: %s\n,
+ kretprobe_blacklist[i].name);
+ }
+ }
+ kprobe_blacklist_initialized = 1;
+}
+
#ifdef __ARCH_WANT_KPROBES_INSN_SLOT
/*
* kprobe-ainsn.insn points to the copy of the instruction to be
@@ -1331,6 +1379,9 @@ static int __kprobes in_kprobes_functions(unsigned
long addr)
if (addr = (unsigned long)__kprobes_text_start
addr (unsigned long)__kprobes_text_end)
return -EINVAL;
+
+ if (!kprobe_blacklist_initialized)
+ init_kprobe_blacklist();
/*
* If there exists a kprobe_blacklist, verify and
* fail any probe registration in the prohibited area
@@ -1816,6 +1867,8 @@ int __kprobes register_kretprobe(struct kretprobe *rp)
void *addr;
if (kretprobe_blacklist_size) {
+ if (!kprobe_blacklist_initialized)
+ init_kprobe_blacklist();
Joonsoo reminds me that these calling points are not protected by
kprobe_mutex,
thus we have to do something for avoiding concurrent initialization.
Perhaps, the easiest way is to protect init_kprobe_blacklist() by kprobe_mutex
and check kprobe_blacklist_initialized again in the top of that.
Yes, you are right. I had a second look at Joonsoo's patch and I will
add a similar mutex for v2.
-Oskar
--
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 0/4] kprobes: split blacklist into common and arch
On 13:32 Thu 04 Apr , Vineet Gupta wrote: Hi Oskar, On 04/03/2013 01:58 PM, oskar.and...@sonymobile.com wrote: Hi, This is a slight rework of the following patches which I posted earlier: [PATCH] Kprobes blacklist: Conditionally add x86-specific symbols [PATCH] delay blacklist symbol lookup until we actually need it This serie includes a patch that moves the architecture dependent black points to the arch/ directory and the x86 patch was modified to follow that patch. -Oskar Björn Davidsson (1): kprobes: move x86-specific blacklist symbols to arch directory Oskar Andero (2): kprobes: split blacklist into common and arch kprobes: replace printk with pr_-functions Toby Collett (1): kprobes: delay blacklist symbol lookup until we actually need it arch/arc/kernel/kprobes.c | 3 + arch/arm/kernel/kprobes.c | 2 + arch/avr32/kernel/kprobes.c| 3 + arch/ia64/kernel/kprobes.c | 3 + arch/mips/kernel/kprobes.c | 3 + arch/mn10300/kernel/kprobes.c | 2 + arch/powerpc/kernel/kprobes.c | 3 + arch/s390/kernel/kprobes.c | 3 + arch/sh/kernel/kprobes.c | 3 + arch/sparc/kernel/kprobes.c| 3 + arch/x86/kernel/kprobes/core.c | 7 ++ kernel/kprobes.c | 149 ++--- 12 files changed, 130 insertions(+), 54 deletions(-) For any arch changes, however trivial they might be, you need to CC the respective maintainers, or atleast post the patches on linux-arch so we know what's going on ! Thanks Vineet! I will make sure to add linux-arch on CC for version 2. -Oskar -- 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 2/4] kprobes: split blacklist into common and arch
On 08:17 Thu 04 Apr , Masami Hiramatsu wrote: (2013/04/03 17:28), oskar.and...@sonymobile.com wrote: Some blackpoints are only valid for specific architectures. To let each architecture specify its own blackpoints the list has been split in two lists: common and arch. The common list is kept in kernel/kprobes.c and the arch list is kept in the arch/ directory. Hmm, I think you'd better merge this with patch 3/4. IMHO I think it's more logical to keep them separated instead of squashing a generic patch with an architecture specific patch. Also, there is the matter of authorship and credits. Of course, it's your call, so let me know if this is ok with you. [...] +static const size_t common_kprobes_blacksyms_size = + ARRAY_SIZE(common_kprobes_blacksyms); + +extern const char * const arch_kprobes_blacksyms[]; +extern const size_t arch_kprobes_blacksyms_size; + +static struct kprobe_blackpoint **kprobe_blacklist; +static size_t kprobe_blacklist_size; Since the blacklist is allocated once and never be updated, we just need an array of struct kprobe_blackpoint, no need to allocate each entry. Right. I'll change that. Thanks! -Oskar -- 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 v2 1/4] kprobes: delay blacklist symbol lookup until we actually need it
From: Toby Collett toby.coll...@sonymobile.com
The symbol lookup can take a long time and kprobes is
initialised very early in boot, so delay symbol lookup
until the blacklist is first used.
Cc: Masami Hiramatsu masami.hiramatsu...@hitachi.com
Cc: David S. Miller da...@davemloft.net
Reviewed-by: Radovan Lekanovic radovan.lekano...@sonymobile.com
Signed-off-by: Toby Collett toby.coll...@sonymobile.com
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
---
kernel/kprobes.c | 98 ++--
1 file changed, 60 insertions(+), 38 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index e35be53..0a270e5 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -68,6 +68,7 @@
#endif
static int kprobes_initialized;
+static int kprobe_blacklist_initialized;
static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
@@ -102,6 +103,60 @@ static struct kprobe_blackpoint kprobe_blacklist[] = {
{NULL}/* Terminator */
};
+/* it can take some time ( 100ms ) to initialise the
+ * blacklist so we delay this until we actually need it
+ */
+static void init_kprobe_blacklist(void)
+{
+ int i;
+ unsigned long offset = 0, size = 0;
+ char *modname, namebuf[128];
+ const char *symbol_name;
+ void *addr;
+ struct kprobe_blackpoint *kb;
+
+ mutex_lock(kprobe_mutex);
+ if (kprobe_blacklist_initialized)
+ goto out;
+
+ /*
+* Lookup and populate the kprobe_blacklist.
+*
+* Unlike the kretprobe blacklist, we'll need to determine
+* the range of addresses that belong to the said functions,
+* since a kprobe need not necessarily be at the beginning
+* of a function.
+*/
+ for (kb = kprobe_blacklist; kb-name != NULL; kb++) {
+ kprobe_lookup_name(kb-name, addr);
+ if (!addr)
+ continue;
+
+ kb-start_addr = (unsigned long)addr;
+ symbol_name = kallsyms_lookup(kb-start_addr,
+ size, offset, modname, namebuf);
+ if (!symbol_name)
+ kb-range = 0;
+ else
+ kb-range = size;
+ }
+
+ if (kretprobe_blacklist_size) {
+ /* lookup the function address from its name */
+ for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
+ kprobe_lookup_name(kretprobe_blacklist[i].name,
+ kretprobe_blacklist[i].addr);
+ if (!kretprobe_blacklist[i].addr)
+ printk(kretprobe: lookup failed: %s\n,
+ kretprobe_blacklist[i].name);
+ }
+ }
+ kprobe_blacklist_initialized = 1;
+
+out:
+ mutex_unlock(kprobe_mutex);
+}
+
#ifdef __ARCH_WANT_KPROBES_INSN_SLOT
/*
* kprobe-ainsn.insn points to the copy of the instruction to be
@@ -1331,6 +1386,9 @@ static int __kprobes in_kprobes_functions(unsigned long
addr)
if (addr = (unsigned long)__kprobes_text_start
addr (unsigned long)__kprobes_text_end)
return -EINVAL;
+
+ if (unlikely(!kprobe_blacklist_initialized))
+ init_kprobe_blacklist();
/*
* If there exists a kprobe_blacklist, verify and
* fail any probe registration in the prohibited area
@@ -1816,6 +1874,8 @@ int __kprobes register_kretprobe(struct kretprobe *rp)
void *addr;
if (kretprobe_blacklist_size) {
+ if (unlikely(!kprobe_blacklist_initialized))
+ init_kprobe_blacklist();
addr = kprobe_addr(rp-kp);
if (IS_ERR(addr))
return PTR_ERR(addr);
@@ -2065,11 +2125,6 @@ static struct notifier_block kprobe_module_nb = {
static int __init init_kprobes(void)
{
int i, err = 0;
- unsigned long offset = 0, size = 0;
- char *modname, namebuf[128];
- const char *symbol_name;
- void *addr;
- struct kprobe_blackpoint *kb;
/* FIXME allocate the probe table, currently defined statically */
/* initialize all list heads */
@@ -2079,39 +2134,6 @@ static int __init init_kprobes(void)
raw_spin_lock_init((kretprobe_table_locks[i].lock));
}
- /*
-* Lookup and populate the kprobe_blacklist.
-*
-* Unlike the kretprobe blacklist, we'll need to determine
-* the range of addresses that belong to the said functions,
-* since a kprobe need not necessarily be at the beginning
-* of a function.
-*/
- for (kb = kprobe_blacklist; kb-name != NULL; kb++) {
- kprobe_lookup_name(kb-name, addr);
- if (!addr)
- continue;
-
- kb
[PATCH v2 4/4] kprobes: replace printk with pr_-functions
Instead of using printk use pr_info/pr_err/pr_warn. This was
detected by the checkpatch.pl script.
Cc: Masami Hiramatsu masami.hiramatsu...@hitachi.com
Cc: David S. Miller da...@davemloft.net
Signed-off-by: Oskar Andero oskar.and...@sonymobile.com
Acked-by: Masami Hiramatsu masami.hiramatsu...@hitachi.com
---
kernel/kprobes.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 58c9f4e..d61cbad 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -168,7 +168,7 @@ static void init_kprobe_blacklist(void)
kprobe_lookup_name(kretprobe_blacklist[i].name,
kretprobe_blacklist[i].addr);
if (!kretprobe_blacklist[i].addr)
- printk(kretprobe: lookup failed: %s\n,
+ pr_err(kretprobe: lookup failed: %s\n,
kretprobe_blacklist[i].name);
}
}
@@ -780,7 +780,7 @@ static void reuse_unused_kprobe(struct kprobe *ap)
*/
op = container_of(ap, struct optimized_kprobe, kp);
if (unlikely(list_empty(op-list)))
- printk(KERN_WARNING Warning: found a stray unused
+ pr_warn(Warning: found a stray unused
aggrprobe@%p\n, ap-addr);
/* Enable the probe again */
ap-flags = ~KPROBE_FLAG_DISABLED;
@@ -887,7 +887,7 @@ static void __kprobes optimize_all_kprobes(void)
if (!kprobe_disabled(p))
optimize_kprobe(p);
}
- printk(KERN_INFO Kprobes globally optimized\n);
+ pr_info(Kprobes globally optimized\n);
}
/* This should be called with kprobe_mutex locked */
@@ -911,7 +911,7 @@ static void __kprobes unoptimize_all_kprobes(void)
}
/* Wait for unoptimizing completion */
wait_for_kprobe_optimizer();
- printk(KERN_INFO Kprobes globally unoptimized\n);
+ pr_info(Kprobes globally unoptimized\n);
}
int sysctl_kprobes_optimization;
@@ -982,7 +982,7 @@ static void __kprobes __disarm_kprobe(struct kprobe *p,
bool reopt)
/* There should be no unused kprobes can be reused without optimization */
static void reuse_unused_kprobe(struct kprobe *ap)
{
- printk(KERN_ERR Error: There should be no unused kprobe here.\n);
+ pr_err(Error: There should be no unused kprobe here.\n);
BUG_ON(kprobe_unused(ap));
}
@@ -2096,8 +2096,8 @@ EXPORT_SYMBOL_GPL(enable_kprobe);
void __kprobes dump_kprobe(struct kprobe *kp)
{
- printk(KERN_WARNING Dumping kprobe:\n);
- printk(KERN_WARNING Name: %s\nAddress: %p\nOffset: %x\n,
+ pr_warn(Dumping kprobe:\n);
+ pr_warn(Name: %s\nAddress: %p\nOffset: %x\n,
kp-symbol_name, kp-addr, kp-offset);
}
@@ -2293,7 +2293,7 @@ static void __kprobes arm_all_kprobes(void)
}
kprobes_all_disarmed = false;
- printk(KERN_INFO Kprobes globally enabled\n);
+ pr_info(Kprobes globally enabled\n);
already_enabled:
mutex_unlock(kprobe_mutex);
@@ -2315,7 +2315,7 @@ static void __kprobes disarm_all_kprobes(void)
}
kprobes_all_disarmed = true;
- printk(KERN_INFO Kprobes globally disabled\n);
+ pr_info(Kprobes globally disabled\n);
for (i = 0; i KPROBE_TABLE_SIZE; i++) {
head = kprobe_table[i];
--
1.8.1.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/
