[PATCH] tty: fix crash in release_tty if tty->port is not set
Commit 2ae0b31e0face ("tty: don't crash in tty_init_dev when missing
tty_port") didn't fully prevent the crash as the cleanup path in
tty_init_dev() calls release_tty() which dereferences tty->port
without checking it for non-null.
Add tty->port checks to release_tty to avoid the kernel crash.
Fixes: 2ae0b31e0face ("tty: don't crash in tty_init_dev when missing tty_port")
Signed-off-by: Matthias Reichl
---
drivers/tty/tty_io.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 7a4c02548fb3f..9f8b9a567b359 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1515,10 +1515,12 @@ static void release_tty(struct tty_struct *tty, int idx)
tty->ops->shutdown(tty);
tty_save_termios(tty);
tty_driver_remove_tty(tty->driver, tty);
- tty->port->itty = NULL;
+ if (tty->port)
+ tty->port->itty = NULL;
if (tty->link)
tty->link->port->itty = NULL;
- tty_buffer_cancel_work(tty->port);
+ if (tty->port)
+ tty_buffer_cancel_work(tty->port);
if (tty->link)
tty_buffer_cancel_work(tty->link->port);
--
2.20.1
Re: Crash when specifying non-existent serial port in speakup / tty_kopen
On Wed, Nov 04, 2020 at 10:30:05PM +0100, Samuel Thibault wrote: > Matthias Reichl, le mer. 04 nov. 2020 22:15:05 +0100, a ecrit: > > > This looks like only a warning, did it actually crash? > > > > Yes, scroll down a bit, the null pointer oops followed almost > > immediately after that > > > > [ 49.979043] BUG: kernel NULL pointer dereference, address: > > 0090 > > Ah, [ 50.102938] tty_init_dev+0xb5/0x1d0 > > probably the trailing release_tty call that does > > tty->port->itty = NULL; > (itty is after a struct tty_bufhead + the tty pointer, that looks > plausible). > > so probably an if (tty->port) in release_tty could help? Thanks a lot, good catch! This is indeed where the crash happens and checking for tty->port in release_tty() prevents that. I'll send a patch. so long, Hias
Re: Crash when specifying non-existent serial port in speakup / tty_kopen
Hi Samuel, On Wed, Nov 04, 2020 at 09:13:23PM +0100, Samuel Thibault wrote: > Hello, > > Matthias Reichl, le mer. 04 nov. 2020 15:57:37 +0100, a ecrit: > > I initially noticed this oops on x86_64 running kernel 5.4.59 when > > I accidentally mistyped "ttyS0" as "ttyS9": > > > > modprobe speakup_dummy dev=ttyS9 > > > [ 49.978481] tty_init_dev: ttyS driver does not set tty->port. This would > > crash the kernel. Fix the driver! > > This looks like only a warning, did it actually crash? Yes, scroll down a bit, the null pointer oops followed almost immediately after that [ 49.979043] BUG: kernel NULL pointer dereference, address: 0090 > > the missing tty->port is quite fatal. > > It is fatal for module insertion yes (EINVAL) but IIRC that should be > getting handled properly, making modprobe return the error? When I did that on my desktop the tty system was pretty screwed. Mouse still worked in X but no keyboard input possible. > > It looks like spk_ttyio or tty_dev_name_to_number() / tty_kopen() > > should perform some additional validation, > > spk_ttyio_initialise_ldisc only has a dev_t so can't do much beyond > calling tty_kopen. > > tty_kopen is getting the index from the tty_lookup_driver call (actually > get_tty_driver which uses p->minor_start and p->num) and passes it to > tty_driver_lookup_tty. Perhaps in addition of p->num the driver should > have another field to set, that tty_init_dev could use to reject with > ENODEV indexes beyond what the driver actually provides? It might be a bit more involved than a simple max port check, think about hotplug (I have 16C950 ExpressCard devices I occansionally use on one of my laptops) so there may be holes in the allocation numbers. Not sure how/where to best solve this. > > I couldn't make the kernel warn/crash yet by specifying non-existent > > ttyUSB ports yet though. > > That's probably because in the ttyUSB case the device allocation is > dynamic and made exactly according to the number of actual devices, > while for ttyS* there is a large overcommit of minor values. Yes, that sounds reasonable (haven't looked at ttyUSB details, only checked serial core devices yet). so long, Hias
Crash when specifying non-existent serial port in speakup / tty_kopen
I initially noticed this oops on x86_64 running kernel 5.4.59 when I accidentally mistyped "ttyS0" as "ttyS9": modprobe speakup_dummy dev=ttyS9 x86_64/5.10-rc2 showed the same behaviour (see below), also 5.9.3 on RPi with the ttyAMA driver. I couldn't make the kernel warn/crash yet by specifying non-existent ttyUSB ports yet though. It looks like spk_ttyio or tty_dev_name_to_number() / tty_kopen() should perform some additional validation, as the missing tty->port is quite fatal. Here's the I got on 5.10-rc2: [ 49.967409] input: Speakup as /devices/virtual/input/input10 [ 49.967731] initialized device: /dev/synth, node (MAJOR 10, MINOR 61) [ 49.968848] speakup 3.1.6: initialized [ 49.968852] synth name on entry is: (null) [ 49.978421] synth probe [ 49.978477] [ cut here ] [ 49.978481] tty_init_dev: ttyS driver does not set tty->port. This would crash the kernel. Fix the driver! [ 49.978522] WARNING: CPU: 1 PID: 283 at drivers/tty/tty_io.c:1351 tty_init_dev+0x17a/0x1d0 [ 49.978525] Modules linked in: speakup_dummy(+) speakup [ 49.978538] CPU: 1 PID: 283 Comm: modprobe Not tainted 5.10.0-rc2 #5 [ 49.978541] Hardware name: /8IPE775/-G, BIOS F5 08/31/2006 [ 49.978580] RIP: 0010:tty_init_dev+0x17a/0x1d0 [ 49.978586] Code: ff ff e8 f9 a6 f5 ff 85 c0 0f 84 43 ff ff ff 49 8b 46 10 48 c7 c6 98 30 07 82 48 c7 c7 b0 c0 26 82 48 8b 50 20 e8 95 37 69 00 <0f> 0b e9 21 ff ff ff e8 da da ff ff e9 ed fe ff ff 4c 89 f7 89 45 [ 49.978591] RSP: 0018:c929bb88 EFLAGS: 00010282 [ 49.978596] RAX: RBX: ffea RCX: 0027 [ 49.978599] RDX: 0027 RSI: 88807dc97820 RDI: 88807dc97828 [ 49.978603] RBP: c929bbb0 R08: R09: c000dfff [ 49.978605] R10: 0001 R11: c929b958 R12: 88800386df00 [ 49.978608] R13: 0009 R14: 88800514c000 R15: a001e500 [ 49.978613] FS: 7faebf8ae540() GS:88807dc8() knlGS: [ 49.978617] CS: 0010 DS: ES: CR0: 80050033 [ 49.978621] CR2: 7fff4ff25ff8 CR3: 0529a000 CR4: 06e0 [ 49.978624] Call Trace: [ 49.978635] tty_kopen+0x101/0x150 [ 49.978651] spk_ttyio_synth_probe+0xd5/0x230 [speakup] [ 49.978661] ? _cond_resched+0x14/0x30 [ 49.978669] ? do_init_module+0x22/0x200 [ 49.978678] do_synth_init.cold.11+0x2d/0x15f [speakup] [ 49.978686] synth_add+0x95/0xa0 [speakup] [ 49.978689] ? 0xa0021000 [ 49.978696] synth_dummy_init+0x10/0x1000 [speakup_dummy] [ 49.978702] do_one_initcall+0x45/0x1d0 [ 49.978707] ? _cond_resched+0x14/0x30 [ 49.978714] ? kmem_cache_alloc_trace+0x39/0x1b0 [ 49.978720] do_init_module+0x5b/0x200 [ 49.978726] load_module+0x25e6/0x2830 [ 49.978733] __do_sys_finit_module+0xc1/0x120 [ 49.978737] ? __do_sys_finit_module+0xc1/0x120 [ 49.978744] __x64_sys_finit_module+0x15/0x20 [ 49.978750] do_syscall_64+0x37/0x50 [ 49.978757] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 49.978762] RIP: 0033:0x7faebf9cf919 [ 49.978767] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 47 55 0c 00 f7 d8 64 89 01 48 [ 49.978771] RSP: 002b:7fffb3a03018 EFLAGS: 0246 ORIG_RAX: 0139 [ 49.978777] RAX: ffda RBX: 5620ea9c58f0 RCX: 7faebf9cf919 [ 49.978780] RDX: RSI: 5620ea9c5e10 RDI: 0004 [ 49.978783] RBP: 0004 R08: R09: [ 49.978785] R10: 0004 R11: 0246 R12: 5620ea9c5e10 [ 49.978788] R13: R14: 5620ea9c5ab0 R15: 5620ea9c58f0 [ 49.978795] CPU: 1 PID: 283 Comm: modprobe Not tainted 5.10.0-rc2 #5 [ 49.978798] Hardware name: /8IPE775/-G, BIOS F5 08/31/2006 [ 49.978800] Call Trace: [ 49.978806] dump_stack+0x5e/0x74 [ 49.978813] __warn.cold.13+0xe/0x3f [ 49.978819] ? tty_init_dev+0x17a/0x1d0 [ 49.978826] report_bug+0xc5/0x100 [ 49.978831] handle_bug+0x48/0x90 [ 49.978835] exc_invalid_op+0x18/0x70 [ 49.978839] asm_exc_invalid_op+0x12/0x20 [ 49.978844] RIP: 0010:tty_init_dev+0x17a/0x1d0 [ 49.978849] Code: ff ff e8 f9 a6 f5 ff 85 c0 0f 84 43 ff ff ff 49 8b 46 10 48 c7 c6 98 30 07 82 48 c7 c7 b0 c0 26 82 48 8b 50 20 e8 95 37 69 00 <0f> 0b e9 21 ff ff ff e8 da da ff ff e9 ed fe ff ff 4c 89 f7 89 45 [ 49.978852] RSP: 0018:c929bb88 EFLAGS: 00010282 [ 49.978857] RAX: RBX: ffea RCX: 0027 [ 49.978860] RDX: 0027 RSI: 88807dc97820 RDI: 88807dc97828 [ 49.978863] RBP: c929bbb0 R08: R09: c000dfff [ 49.978865] R10: 0001 R11: c929b958 R12: 88800386df00 [ 49.978868] R13: 0009 R14: 88800514c000 R15: a001e500 [
Re: [PATCH] ASoC: bcm2835: Add enable/disable clock functions
On Wed, Oct 28, 2020 at 01:18:33AM -0700, Allen Martin wrote:
> Hi, just checking if you had a chance to review this patch.
>
> On Sat, Oct 10, 2020 at 12:26 PM Allen Martin wrote:
>
> > Add functions to control enable/disable of BCLK output of bcm2835 I2S
> > controller so that BCLK output only starts when dma starts. This
> > resolves issues of audio pop with DACs such as max98357 on rpi. The
> > LRCLK output of bcm2835 only starts when the frame size has been
> > configured and there is data in the FIFO. The max98357 dac makes a
> > loud popping sound when BCLK is toggling but LRCLK is not.
I'm afraid that changing the clocking in the way you proposed has a high
potential of breaking existing setups which need bclk to be present
after prepare(). And it complicates the already rather convoluted
clock setup even more. So I don't think this patch should be applied.
Since you mentioned max98357: have you properly connected and configured
the sd-mode GPIO? This chip has very strict timing requirements and is
known to "pop" without sd-mode wired up - see the datasheet and devicetree
binding docs.
so long,
Hias
> >
> > Signed-off-by: Allen Martin
> > ---
> > sound/soc/bcm/bcm2835-i2s.c | 35 +++
> > 1 file changed, 35 insertions(+)
> >
> > diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c
> > index e6a12e271b07..5c8649864c0d 100644
> > --- a/sound/soc/bcm/bcm2835-i2s.c
> > +++ b/sound/soc/bcm/bcm2835-i2s.c
> > @@ -122,9 +122,27 @@ struct bcm2835_i2s_dev {
> > struct regmap *i2s_regmap;
> > struct clk *clk;
> > boolclk_prepared;
> > + boolclk_enabled;
> > int clk_rate;
> > };
> >
> > +static void bcm2835_i2s_enable_clock(struct bcm2835_i2s_dev *dev)
> > +{
> > + if (dev->clk_enabled)
> > + return;
> > +
> > + regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_MODE_A_REG,
> > BCM2835_I2S_CLKDIS, 0);
> > + dev->clk_enabled = true;
> > +}
> > +
> > +static void bcm2835_i2s_disable_clock(struct bcm2835_i2s_dev *dev)
> > +{
> > + if (dev->clk_enabled)
> > + regmap_update_bits(dev->i2s_regmap,
> > BCM2835_I2S_MODE_A_REG, BCM2835_I2S_CLKDIS, BCM2835_I2S_CLKDIS);
> > +
> > + dev->clk_enabled = false;
> > +}
> > +
> > static void bcm2835_i2s_start_clock(struct bcm2835_i2s_dev *dev)
> > {
> > unsigned int master = dev->fmt & SND_SOC_DAIFMT_MASTER_MASK;
> > @@ -145,6 +163,7 @@ static void bcm2835_i2s_start_clock(struct
> > bcm2835_i2s_dev *dev)
> >
> > static void bcm2835_i2s_stop_clock(struct bcm2835_i2s_dev *dev)
> > {
> > + bcm2835_i2s_disable_clock(dev);
> > if (dev->clk_prepared)
> > clk_disable_unprepare(dev->clk);
> > dev->clk_prepared = false;
> > @@ -158,6 +177,7 @@ static void bcm2835_i2s_clear_fifos(struct
> > bcm2835_i2s_dev *dev,
> > uint32_t csreg;
> > uint32_t i2s_active_state;
> > bool clk_was_prepared;
> > + bool clk_was_enabled;
> > uint32_t off;
> > uint32_t clr;
> >
> > @@ -176,6 +196,11 @@ static void bcm2835_i2s_clear_fifos(struct
> > bcm2835_i2s_dev *dev,
> > if (!clk_was_prepared)
> > bcm2835_i2s_start_clock(dev);
> >
> > + /* Enable clock if not enabled */
> > + clk_was_enabled = dev->clk_enabled;
> > + if (!clk_was_enabled)
> > + bcm2835_i2s_enable_clock(dev);
> > +
> > /* Stop I2S module */
> > regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, off, 0);
> >
> > @@ -207,6 +232,10 @@ static void bcm2835_i2s_clear_fifos(struct
> > bcm2835_i2s_dev *dev,
> > if (!timeout)
> > dev_err(dev->dev, "I2S SYNC error!\n");
> >
> > + /* Disable clock if it was not enabled before */
> > + if (!clk_was_enabled)
> > + bcm2835_i2s_disable_clock(dev);
> > +
> > /* Stop clock if it was not running before */
> > if (!clk_was_prepared)
> > bcm2835_i2s_stop_clock(dev);
> > @@ -414,6 +443,8 @@ static int bcm2835_i2s_hw_params(struct
> > snd_pcm_substream *substream,
> > /* Clock should only be set up here if CPU is clock master */
> > if (bit_clock_master &&
> > (!dev->clk_prepared || dev->clk_rate != bclk_rate)) {
> > + if (dev->clk_enabled)
> > + bcm2835_i2s_disable_clock(dev);
> > if (dev->clk_prepared)
> > bcm2835_i2s_stop_clock(dev);
> >
> > @@ -534,6 +565,8 @@ static int bcm2835_i2s_hw_params(struct
> > snd_pcm_substream *substream,
> > mode |= BCM2835_I2S_FTXP | BCM2835_I2S_FRXP;
> > }
> >
> > + if (!dev->clk_enabled)
> > + mode |= BCM2835_I2S_CLKDIS;
> > mode |= BCM2835_I2S_FLEN(frame_leng
Re: [alsa-devel] [PATCH] ASoC: soc-core: Fix null pointer dereference in soc_find_component
On Tue, Jan 15, 2019 at 09:41:38PM +, Mark Brown wrote: > On Tue, Jan 15, 2019 at 03:16:57PM -0600, Pierre-Louis Bossart wrote: > > > > Maybe the defer card probe logic needs to be extended to also check if > > > dai_link_name had already been registered (either cpu or cpu_dai_name > > > needs to be set), not 100% sure which problem the defer card probe patch > > > was trying to solve. > > We were getting cards probing without the platforms being registered > (which in turn meant we were just skipping their init) and had patches > proposed to implement the deferral in the cards. The deferral stuff is > supposed to making sure that everything is registered when we > instantiate. Ah, that makes sense. Thanks a lot for the info! so long, Hias > > > same here, I don't get why the deferred probe stuff only deals with one of > > the two options. > > I think it's just an oversight - I think the change you were proposing > to check the cpu_dai_name is a good idea anyway as it makes things more > consistent and work more obviously by intention. And more generally if > we can simplify the code by removing legacy options that'd be good but > that's a bigger bit of work...
Re: [alsa-devel] [PATCH] ASoC: soc-core: Fix null pointer dereference in soc_find_component
On Tue, Jan 15, 2019 at 01:35:07PM -0600, Pierre-Louis Bossart wrote:
>
> On 1/14/19 6:06 PM, Mark Brown wrote:
> > On Fri, Jan 11, 2019 at 03:49:08PM -0600, Pierre-Louis Bossart wrote:
> >
> > > Adding some traces I can see that the the platform name we use doesn't
> > > seem
> > > compatible with your logic. All the Intel boards used a constant platform
> > > name matching the PCI ID, see e.g. [1], which IIRC is used to bind
> > > components. Liam, do you recall in more details if this is really
> > > required?
> > That's telling me that either snd_soc_find_components() isn't finding
> > components in the same way that we do when we bind things which isn't
> > good or we're binding links without having fully matched everything on
> > the link which also isn't good.
> >
> > Without a system that shows the issue I can't 100% confirm but I think
> > it's the former - I'm fairly sure that those machines are relying on the
> > component name being initialized to fmt_single_name() in
> > snd_soc_component_initialize(). That is supposed to wind up using
> > dev_name() (which would be the PCI address for a PCI device) as the
> > basis of the name. What I can't currently see is how exactly that gets
> > bound (or how any of the other links avoid trouble for that matter). We
> > could revert and push this into cards but I would rather be confident
> > that we understand what's going on, I'm not comfortable that we aren't
> > just pushing the breakage around rather than fixing it. Can someone
> > with an x86 system take a look and confirm exactly what's going on with
> > binding these cards please?
>
> Beyond the fact that the platform_name seems to be totally useless,
> additional tests show that the patch ('ASoC: soc-core: defer card probe
> until all component is added to list') adds a new restriction which
> contradicts existing error checks.
>
> None of the Intel machine drivers set the dailink "cpu_name" field but use
> the "cpu_dai_name" field instead. This was perfectly legit as documented by
> the code at the end of soc_init_dai_link()
This should be fixed by the patch
"ASoC: core: Don't defer probe on optional, NULL components" which Mark
already applied to his tree. See
http://mailman.alsa-project.org/pipermail/alsa-devel/2019-January/144323.html
Maybe the defer card probe logic needs to be extended to also check if
dai_link_name had already been registered (either cpu or cpu_dai_name
needs to be set), not 100% sure which problem the defer card probe patch
was trying to solve.
so long,
Hias
>
> /*
> * At least one of CPU DAI name or CPU device name/node must be
> * specified
> */
> if (!link->cpu_dai_name &&
> !(link->cpu_name || link->cpu_of_node)) {
> dev_err(card->dev,
> "ASoC: Neither cpu_dai_name nor cpu_name/of_node are set for
> %s\n",
> link->name);
> return -EINVAL;
> }
>
> The code contributed by Qualcomm only checks for cpu_name, which prevents
> the init from completing.
>
> So if we want to be consistent, the new code should be something like:
>
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index b680c673c553..2791da9417f8 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -1154,7 +1154,7 @@ static int soc_init_dai_link(struct snd_soc_card
> *card,
> * Defer card registartion if cpu dai component is not added to
> * component list.
> */
> - if (!soc_find_component(link->cpu_of_node, link->cpu_name))
> + if (!link->cpu_dai_name && !soc_find_component(link->cpu_of_node,
> link->cpu_name))
> return -EPROBE_DEFER;
>
> /*
>
> or try to call soc_find_component with both cpu_name or cpu_dai_name, if
> this makes sense?
> ___
> Alsa-devel mailing list
> alsa-de...@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Re: [alsa-devel] [PATCH v1 3/3] ASoC: soc-core: fix platform name vs. of_node assignement
On Tue, Jan 15, 2019 at 03:26:42PM +, Jon Hunter wrote: > > On 14/01/2019 23:02, Mark Brown wrote: > > On Fri, Jan 11, 2019 at 09:15:42AM +, Jon Hunter wrote: > >> On 11/01/2019 08:51, Kuninori Morimoto wrote: > > > >>> So, can you agree about these ? > >>> 1) Add enough information/documentation about legacy/modern style and its > >>> plan. > >>> 2) Add dirty pointer fixup patch as workaround > >>> 3) switch to modern style as much as possible > > > >> I think that Mark needs to decided on whether use your 'dirty pointer' > >> fix or not. > > > > I've gone ahead with Curtis' version of Morimoto-san's patch just now - > > hopefully that's fine for v5.0. Like we've been saying neither approach > > is ideal, thanks Jon and Morimoto-san for your efforts analyzing this > > and your proposals. > > Thanks! Works for me. Thanks alot, works here, too on 4.20 and 5.0-rc2! so long, Hias
Re: [alsa-devel] [PATCH v1 3/3] ASoC: soc-core: fix platform name vs. of_node assignement
Hi Mark, On Sun, Oct 21, 2018 at 12:23:01PM +0100, Mark Brown wrote: > On Fri, Oct 19, 2018 at 11:22:46AM +0100, Jon Hunter wrote: > > > Looking at snd_soc_init_platform(), it seems that the platform pointer > > can be allocated by the machine driver and so if it is not allocated by > > the core, then I don't think we should clear it here. Seems we need a > > way to determine if this was allocated by the core. > > Indeed, this is a bit of a mess. We probably shouldn't be modifying the > data that the drivers passed in, otherwise we get into trouble like > this. That suggests that we should copy the data, probably all of it. > I will try to have a proper look at this next week. did you find the time to look into this? The downstream Raspberry Pi kernel contains a bunch of machine drivers that are implemented in a similar way as the tegra_sgtl5000 driver (static card and dai link structs, dai_link->platform_of_node filled in from device tree) which are breaking in 4.20 on deferred probing. Switching these drivers to dynamically allocated dai link structs, like 76836fd35492 "ASoC: omap-abe-twl6040: Fix missing audio card caused by deferred probing" would be a possibility, but if there's some solution on the horizon that doesn't require changes to the driver code it'd be easier to wait for that. so long, Hias > > Furthermore, it seems that it is possible that there is more than one > > link that might be to be cleared. > > Yes, that's an issue as well. > ___ > Alsa-devel mailing list > alsa-de...@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Add "regulator: arizona-ldo1: Use correct device to get enable GPIO" to 4.18 stable tree
It seems the arizona-ldo1 ldoena fix hasn't made it into 4.18. It was added to 4.19/mainline, though: commit a9191579ba1086d91842199263e6fe6bb5eec1ba Author: Charles Keepax Date: Tue Jun 19 16:10:00 2018 +0100 regulator: arizona-ldo1: Use correct device to get enable GPIO Could you please add this commit to the 4.18 stable queue? Without this fix LDO control via GPIO is broken and Arizona devices can't be used as no power is applied. I tested locally with a WM5102 (Cirrus Logic Audio Card on RPi), with stock 4.18 tree device detection fails: [6.075981] arizona spi0.1: Unknown device ID: 0 with this commit added to 4.18 the WM5102 is detected fine: [6.060887] arizona spi0.1: WM5102 revision C so long & thanks, Hias
[PATCH] kbuild: honor HOSTCFLAGS and HOSTLDFLAGS in libelf test
Since commit 596a9f6768af ("objtool: Support HOSTCFLAGS and HOSTLDFLAGS")
objtool can use a libelf installed in a non-standard location by
passing in appropriate HOST flags.
The libelf check in the main Makefile is done without these flags
and fails if no libelf is installed on the system.
Fix this by adding HOSTCFLAGS and HOSTLDFLAGS to the libelf check.
Signed-off-by: Matthias Reichl
---
Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index c9132594860b..b0485fddc554 100644
--- a/Makefile
+++ b/Makefile
@@ -933,7 +933,7 @@ export mod_sign_cmd
ifdef CONFIG_STACK_VALIDATION
has_libelf := $(call try-run,\
- echo "int main() {}" | $(HOSTCC) -xc -o /dev/null -lelf -,1,0)
+ echo "int main() {}" | $(HOSTCC) $(HOSTCFLAGS) -xc -o /dev/null
-lelf $(HOSTLDFLAGS) -,1,0)
ifeq ($(has_libelf),1)
objtool_target := tools/objtool FORCE
else
--
2.11.0
Re: [PATCH] Arm: mm: ftrace: Only set text back to ro after kernel has been marked ro
On Fri, Jun 29, 2018 at 11:16:58AM -0400, Steven Rostedt wrote:
> On Fri, 29 Jun 2018 16:47:14 +0200
> Matthias Reichl wrote:
>
> > On Tue, Dec 05, 2017 at 12:14:46PM -0800, Kees Cook wrote:
> > > On Tue, Dec 5, 2017 at 12:09 PM, Russell King - ARM Linux
> > > wrote:
> > > > On Tue, Dec 05, 2017 at 11:35:59AM -0800, Kees Cook wrote:
> > > >> We don't _need_ to, but they're all contiguous, so the ro_perms array
> > > >> used by set_kernel_text_*() is actually only a single entry:
> > > >>
> > > >> static struct section_perm ro_perms[] = {
> > > >> /* Make kernel code and rodata RX (set RO). */
> > > >> {
> > > >> .name = "text/rodata RO",
> > > >> .start = (unsigned long)_stext,
> > > >> .end= (unsigned long)__init_begin,
> > > >> ...
> > > >
> > > > Well, they may not be contiguous - it depends on DEBUG_ALIGN_RODATA.
> > >
> > > Maybe I'm picking a slightly wrong word. I guess I meant adjacent. The
> > > range _stext to __init_begin is all read-only, though there may be
> > > padding (controlled by DEBUG_ALIGN_RODATA), to allow a split for NX
> > > markings on rodata.
> > >
> > > > Either way, we have __start_rodata_section_aligned, which is either
> > > > the start of the read-only data section, or the start of the first
> > > > section beyond __start_rodata if DEBUG_ALIGN_RODATA is not set.
> > > >
> > > > Given that __start_rodata_section_aligned will always be less than
> > > > __init_begin, is there any reason not to make the above end at
> > > > __start_rodata_section_aligned, thereby allowing more of the read-only
> > > > data (in the case of DEBUG_ALIGN_RODATA=n) or all of the read-only
> > > > data (in the case of DEBUG_ALIGN_RODATA=y) to remain write-protected?
> > >
> > > Sure, there's no reason not to split this into two entries. It'll
> > > require some reworking of the function calls to get it right,
> > > obviously.
> >
> > Gentle ping, arm is still oopsing when the function tracer is
> > enabled at boot time.
> >
>
> I take it that my patch never got applied:
>
> http://lkml.kernel.org/r/20180621124710.453ee...@gandalf.local.home
Yes, sorry, forgot to include this info in my mail.
Your patch no longer applies cleanly - a8e53c151fe7a added a
debug_checkwx() to mark_rodata_ro() - but when applying it manually
it still fixes the oops.
so long,
Hias
>
> -- Steve
>
>
> > Tested on bcm2835 (RPiB+) with current mainline tree
> > (githash 90368a37fbbe) and bcm2835_defconfig.
> >
> > arm64 seems to be fine, tested on bcm2837 (RPi3) with same tree and
> > arm64 defconfig plus function tracer enabled.
>
Re: [PATCH] Arm: mm: ftrace: Only set text back to ro after kernel has been marked ro
On Tue, Dec 05, 2017 at 12:14:46PM -0800, Kees Cook wrote:
> On Tue, Dec 5, 2017 at 12:09 PM, Russell King - ARM Linux
> wrote:
> > On Tue, Dec 05, 2017 at 11:35:59AM -0800, Kees Cook wrote:
> >> We don't _need_ to, but they're all contiguous, so the ro_perms array
> >> used by set_kernel_text_*() is actually only a single entry:
> >>
> >> static struct section_perm ro_perms[] = {
> >> /* Make kernel code and rodata RX (set RO). */
> >> {
> >> .name = "text/rodata RO",
> >> .start = (unsigned long)_stext,
> >> .end= (unsigned long)__init_begin,
> >> ...
> >
> > Well, they may not be contiguous - it depends on DEBUG_ALIGN_RODATA.
>
> Maybe I'm picking a slightly wrong word. I guess I meant adjacent. The
> range _stext to __init_begin is all read-only, though there may be
> padding (controlled by DEBUG_ALIGN_RODATA), to allow a split for NX
> markings on rodata.
>
> > Either way, we have __start_rodata_section_aligned, which is either
> > the start of the read-only data section, or the start of the first
> > section beyond __start_rodata if DEBUG_ALIGN_RODATA is not set.
> >
> > Given that __start_rodata_section_aligned will always be less than
> > __init_begin, is there any reason not to make the above end at
> > __start_rodata_section_aligned, thereby allowing more of the read-only
> > data (in the case of DEBUG_ALIGN_RODATA=n) or all of the read-only
> > data (in the case of DEBUG_ALIGN_RODATA=y) to remain write-protected?
>
> Sure, there's no reason not to split this into two entries. It'll
> require some reworking of the function calls to get it right,
> obviously.
Gentle ping, arm is still oopsing when the function tracer is
enabled at boot time.
Tested on bcm2835 (RPiB+) with current mainline tree
(githash 90368a37fbbe) and bcm2835_defconfig.
arm64 seems to be fine, tested on bcm2837 (RPi3) with same tree and
arm64 defconfig plus function tracer enabled.
earlycon log on bvm2835:
[0.00] Booting Linux on physical CPU 0x0
[0.00] Linux version 4.18.0-rc2+ (hias@camel2) (gcc version 4.9.3
(crosstool-NG crosstool-ng-1.22.0-88-g8460611)) #1 Fri Jun 29 10:55:21 CEST 2018
[0.00] CPU: ARMv6-compatible processor [410fb767] revision 7 (ARMv7),
cr=00c5387d
[0.00] CPU: PIPT / VIPT nonaliasing data cache, VIPT nonaliasing
instruction cache
[0.00] OF: fdt: Machine model: Raspberry Pi Model B Plus Rev 1.2
[0.00] earlycon: pl11 at MMIO32 0x20201000 (options '')
[0.00] bootconsole [pl11] enabled
[0.00] Memory policy: Data cache writeback
[0.00] cma: Reserved 32 MiB at 0x13c0
[0.00] CPU: All CPU(s) started in SVC mode.
[0.00] random: get_random_bytes called from start_kernel+0x88/0x464
with crng_init=0
[0.00] Built 1 zonelists, mobility grouping on. Total pages: 89408
[0.00] Kernel command line: bcm2708_fb.fbwidth=1280
bcm2708_fb.fbheight=1024 bcm2708_fb.fbswap=1 dma.dmachans=0x7f35
bcm2708.boardrev=0x10 bcm2708.serial=0x59ce1e57 bcm2708.uart_clock=4800
bcm2708.disk_led_gpio=47 bcm2708.disk_led_active_low=0
smsc95xx.macaddr=B8:27:EB:CE:1E:57 vc_mem.mem_base=0x1ec0
vc_mem.mem_size=0x2000 root=/dev/mmcblk0p2 rw rootwait elevator=noop
earlycon=pl011,mmio32,0x20201000 console=/dev/ttyAMA0,115200 ftrace=function
[0.00] Dentry cache hash table entries: 65536 (order: 6, 262144 bytes)
[0.00] Inode-cache hash table entries: 32768 (order: 5, 131072 bytes)
[0.00] Memory: 311736K/360448K available (7168K kernel code, 610K
rwdata, 2328K rodata, 1024K init, 675K bss, 15944K reserved, 32768K
cma-reserved)
[0.00] Virtual kernel memory layout:
[0.00] vector : 0x - 0x1000 ( 4 kB)
[0.00] fixmap : 0xffc0 - 0xfff0 (3072 kB)
[0.00] vmalloc : 0xd680 - 0xff80 ( 656 MB)
[0.00] lowmem : 0xc000 - 0xd600 ( 352 MB)
[0.00] modules : 0xbf00 - 0xc000 ( 16 MB)
[0.00] .text : 0x(ptrval) - 0x(ptrval) (8160 kB)
[0.00] .init : 0x(ptrval) - 0x(ptrval) (1024 kB)
[0.00] .data : 0x(ptrval) - 0x(ptrval) ( 611 kB)
[0.00].bss : 0x(ptrval) - 0x(ptrval) ( 676 kB)
[0.00] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
[0.00] ftrace: allocating 27246 entries in 80 pages
[0.00] Starting tracer 'function'
[0.00] NR_IRQS: 16, nr_irqs: 16, preallocated irqs: 16
[0.54] sched_clock: 32 bits at 1000kHz, resolution 1000ns, wraps every
2147483647500ns
[0.008887] clocksource: timer: mask: 0x max_cycles: 0x,
max_idle_ns: 1911260446275 ns
[0.018977] bcm2835: system timer (irq = 27)
[0.028348] Console: colour dummy device 80x30
[0.033444] Calibrating delay loop... 695.09 BogoMIPS (lpj=3475456)
[0.100506] pid_max: default: 32768 minimum: 301
[0.107059] Unable to hand
Re: [PATCH] regulator: arizona-ldo1: Use correct device to get enable GPIO
On Tue, Jun 19, 2018 at 10:32:45AM +0100, Charles Keepax wrote:
> Currently the enable GPIO is being looked up on the regulator
> device itself but that does not have its own DT node, this causes
> the lookup to fail and the regulator not to get its GPIO. The DT
> node is shared across the whole MFD and as such the lookup needs
> to happen on that parent device. Moving the lookup to the parent
> device also means devres can no longer be used as the life time
> would attach to the wrong device.
>
> Fixes: e1739e86f0cb ("regulator: arizona-ldo1: Look up a descriptor and pass
> to the core")
> Reported-by: Matthias Reichl
> Signed-off-by: Charles Keepax
Tested-by: Matthias Reichl
> Apologies for missing this again, we should have caught
> this in our testing and thanks for reporting it.
No problem, and thank you a lot for the quick fix!
During testing I noticed another minor change introduced by
commit e1739e86f0cb, see comment inline below.
>
> Thanks,
> Charles
>
> drivers/regulator/arizona-ldo1.c | 27 ---
> 1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/regulator/arizona-ldo1.c
> b/drivers/regulator/arizona-ldo1.c
> index f6d6a4ad9e8a..a981896c056a 100644
> --- a/drivers/regulator/arizona-ldo1.c
> +++ b/drivers/regulator/arizona-ldo1.c
> @@ -36,6 +36,8 @@ struct arizona_ldo1 {
>
> struct regulator_consumer_supply supply;
> struct regulator_init_data init_data;
> +
> + struct gpio_desc *ena_gpiod;
> };
>
> static int arizona_ldo1_hc_list_voltage(struct regulator_dev *rdev,
> @@ -253,12 +255,17 @@ static int arizona_ldo1_common_init(struct
> platform_device *pdev,
> }
> }
>
> - /* We assume that high output = regulator off */
> - config.ena_gpiod = devm_gpiod_get_optional(&pdev->dev, "wlf,ldoena",
> -GPIOD_OUT_HIGH);
> + /* We assume that high output = regulator off
> + * Don't use devm, since we need to get against the parent device
> + * so clean up would happen at the wrong time
> + */
> + config.ena_gpiod = gpiod_get_optional(parent_dev, "wlf,ldoena",
> + GPIOD_OUT_HIGH);
The WM5102 datasheet lists LDOENA as an active-high input,
so GPIOD_OUT_HIGH causes arizona-ldo1 to start up enabled.
Not sure about the other devices from the Arizona family.
I did some tests monitoring ldoena and reset gpios with my scope:
With commit e1739e86f0cb reverted ldoena changes from low to high
about 1.6ms before reset changes from low to high.
With your patch ldoena goes high about 11.5ms before reset goes high.
With your patch and GPIOD_OUT_HIGH changed to GPIOD_OUT_LOW
the delay is again about 1.6ms.
So I guess it'd be better to adjust the comment and change
to gpiod flag to GPIOD_OUT_LOW.
so long,
Hias
> if (IS_ERR(config.ena_gpiod))
> return PTR_ERR(config.ena_gpiod);
>
> + ldo1->ena_gpiod = config.ena_gpiod;
> +
> if (pdata->init_data)
> config.init_data = pdata->init_data;
> else
> @@ -276,6 +283,9 @@ static int arizona_ldo1_common_init(struct
> platform_device *pdev,
> of_node_put(config.of_node);
>
> if (IS_ERR(ldo1->regulator)) {
> + if (config.ena_gpiod)
> + gpiod_put(config.ena_gpiod);
> +
> ret = PTR_ERR(ldo1->regulator);
> dev_err(&pdev->dev, "Failed to register LDO1 supply: %d\n",
> ret);
> @@ -334,8 +344,19 @@ static int arizona_ldo1_probe(struct platform_device
> *pdev)
> return ret;
> }
>
> +static int arizona_ldo1_remove(struct platform_device *pdev)
> +{
> + struct arizona_ldo1 *ldo1 = platform_get_drvdata(pdev);
> +
> + if (ldo1->ena_gpiod)
> + gpiod_put(ldo1->ena_gpiod);
> +
> + return 0;
> +}
> +
> static struct platform_driver arizona_ldo1_driver = {
> .probe = arizona_ldo1_probe,
> + .remove = arizona_ldo1_remove,
> .driver = {
> .name = "arizona-ldo1",
> },
> --
> 2.11.0
>
Regression in 4.18-rc1 caused by regulator: arizona-ldo1: Look up a descriptor and pass to the core
Commit e1739e86f0cb9c48e8745a610e6981a4e24cadad breaks reading
the wlf,ldoena property from device tree. This causes ldo1 to
stay off and thus arizona device detection to fail:
[4.495958] of_get_named_gpiod_flags: parsed 'wlf,reset' property of node '/s
oc/spi@7e204000/wm5102@1[0]' - status (0)
[4.509756] arizona-ldo1 arizona-ldo1: GPIO lookup for consumer wlf,ldoena
[4.511339] arizona-ldo1 arizona-ldo1: using lookup tables for GPIO lookup
[4.511351] arizona-ldo1 arizona-ldo1: No GPIO consumer wlf,ldoena found
[4.511413] LDO1: supplied by RPi-Cirrus 1v8
[4.549164] arizona spi0.1: Unknown device ID: 0
With this commit reverted the ldoena GPIO is properly found and
asserted high and the arizona device type is read successfully:
[4.630445] of_get_named_gpiod_flags: parsed 'wlf,reset' property of node '/s
oc/spi@7e204000/wm5102@1[0]' - status (0)
[4.647622] of_get_named_gpiod_flags: parsed 'wlf,ldoena' property of node
'/soc/spi@7e204000/wm5102@1[0]' - status (0)
[4.666100] arizona spi0.1: WM5102 revision C
I've tested this on a Raspberry Pi 3 with upstream 4.18-rc1,
the Cirrus Logic Audio Card from Element 14 (using a WM5102
connected via SPI) and the downstream card driver added as
a module.
Both wlf,reset and wlf,ldoena properties are defined in
the wm5102 devicetree node:
wm5102@1{
compatible = "wlf,wm5102";
reg = <1>;
...
wlf,reset = <&gpio 17 0>;
wlf,ldoena = <&gpio 22 0>;
...
};
If I understand the code correctly the cuplrit seems to be that
with this patch the gpio is looked up from the arizona-ldo1 dev,
which has a NULL of_node:
config.ena_gpiod = devm_gpiod_get_optional(&pdev->dev, "wlf,ldoena", ...
The old version did an of lookup in the parent (arizona) dev, which
contains the of_node with wlf,ldoena:
arizona_ldo1_common_init:
struct device *parent_dev = pdev->dev.parent;
struct regulator_config config = { };
...
config.dev = parent_dev;
...
arizona_ldo1_of_get_pdata(pdata, config, ...
arizona_ldo1_of_get_pdata:
struct device_node *np = config->dev->of_node;
...
pdata->ldoena = of_get_named_gpio(np, "wlf,ldoena", 0);
so long,
Hias
Re: [PATCH v4 0/3] IR decoding using BPF
Hi Sean, On Fri, May 18, 2018 at 03:07:27PM +0100, Sean Young wrote: > The kernel IR decoders (drivers/media/rc/ir-*-decoder.c) support the most > widely used IR protocols, but there are many protocols which are not > supported[1]. For example, the lirc-remotes[2] repo has over 2700 remotes, > many of which are not supported by rc-core. There is a "long tail" of > unsupported IR protocols, for which lircd is need to decode the IR . > > IR encoding is done in such a way that some simple circuit can decode it; > therefore, bpf is ideal. > > In order to support all these protocols, here we have bpf based IR decoding. > The idea is that user-space can define a decoder in bpf, attach it to > the rc device through the lirc chardev. > > Separate work is underway to extend ir-keytable to have an extensive library > of bpf-based decoders, and a much expanded library of rc keymaps. > > Another future application would be to compile IRP[3] to a IR BPF program, and > so support virtually every remote without having to write a decoder for each. > It might also be possible to support non-button devices such as analog > directional pads or air conditioning remote controls and decode the target > temperature in bpf, and pass that to an input device. Thanks a lot, this looks like a very interesting feature to me! Unfortunately I don't have time to test it ATM, but please keep me posted - also on ir-keytable progress - I'm rather excited to give it a try. so long & thanks, Hias > > Thanks, > > Sean Young > > [1] http://www.hifi-remote.com/wiki/index.php?title=DecodeIR > [2] https://sourceforge.net/p/lirc-remotes/code/ci/master/tree/remotes/ > [3] http://www.hifi-remote.com/wiki/index.php?title=IRP_Notation > > Changes since v3: > - Implemented review comments from Quentin Monnet and Y Song (thanks!) > - More helpful and better formatted bpf helper documentation > - Changed back to bpf_prog_array rather than open-coded implementation > - scancodes can be 64 bit > - bpf gets passed values in microseconds, not nanoseconds. >microseconds is more than than enough (IR receivers support carriers upto >70kHz, at which point a single period is already 14 microseconds). Also, >this makes it much more consistent with lirc mode2. > - Since it looks much more like lirc mode2, rename the program type to >BPF_PROG_TYPE_LIRC_MODE2. > - Rebased on bpf-next > > Changes since v2: > - Fixed locking issues > - Improved self-test to cover more cases > - Rebased on bpf-next again > > Changes since v1: > - Code review comments from Y Song and >Randy Dunlap > - Re-wrote sample bpf to be selftest > - Renamed RAWIR_DECODER -> RAWIR_EVENT (Kconfig, context, bpf prog type) > - Rebase on bpf-next > - Introduced bpf_rawir_event context structure with simpler access checking > > Sean Young (3): > bpf: bpf_prog_array_copy() should return -ENOENT if exclude_prog not > found > media: rc: introduce BPF_PROG_LIRC_MODE2 > bpf: add selftest for lirc_mode2 type program > > drivers/media/rc/Kconfig | 13 + > drivers/media/rc/Makefile | 1 + > drivers/media/rc/bpf-lirc.c | 308 ++ > drivers/media/rc/lirc_dev.c | 30 ++ > drivers/media/rc/rc-core-priv.h | 22 ++ > drivers/media/rc/rc-ir-raw.c | 12 +- > include/linux/bpf_rcdev.h | 30 ++ > include/linux/bpf_types.h | 3 + > include/uapi/linux/bpf.h | 53 ++- > kernel/bpf/core.c | 11 +- > kernel/bpf/syscall.c | 7 + > kernel/trace/bpf_trace.c | 2 + > tools/bpf/bpftool/prog.c | 1 + > tools/include/uapi/linux/bpf.h| 53 ++- > tools/include/uapi/linux/lirc.h | 217 > tools/lib/bpf/libbpf.c| 1 + > tools/testing/selftests/bpf/Makefile | 8 +- > tools/testing/selftests/bpf/bpf_helpers.h | 6 + > .../testing/selftests/bpf/test_lirc_mode2.sh | 28 ++ > .../selftests/bpf/test_lirc_mode2_kern.c | 23 ++ > .../selftests/bpf/test_lirc_mode2_user.c | 154 + > 21 files changed, 974 insertions(+), 9 deletions(-) > create mode 100644 drivers/media/rc/bpf-lirc.c > create mode 100644 include/linux/bpf_rcdev.h > create mode 100644 tools/include/uapi/linux/lirc.h > create mode 100755 tools/testing/selftests/bpf/test_lirc_mode2.sh > create mode 100644 tools/testing/selftests/bpf/test_lirc_mode2_kern.c > create mode 100644 tools/testing/selftests/bpf/test_lirc_mode2_user.c > > -- > 2.17.0 >
Re: [PATCH] Arm: mm: ftrace: Only set text back to ro after kernel has been marked ro
On Tue, Dec 05, 2017 at 01:14:17PM +, Russell King - ARM Linux wrote: > On Tue, Dec 05, 2017 at 12:47:09PM +0100, Matthias Reichl wrote: > > On Wed, Aug 23, 2017 at 03:03:51PM -0400, Steven Rostedt wrote: > > > On Wed, 23 Aug 2017 11:48:13 -0700 > > > Kees Cook wrote: > > > > > > > > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c > > > > > index ad80548..fd75f38 100644 > > > > > --- a/arch/arm/mm/init.c > > > > > +++ b/arch/arm/mm/init.c > > > > > @@ -745,19 +745,29 @@ static int __mark_rodata_ro(void *unused) > > > > > return 0; > > > > > } > > > > > > > > > > +static int kernel_set_to_readonly; > > > > > > > > Adding a comment here might be a good idea, something like: > > > > > > > > /* Has system boot-up reached mark_rodata_ro() yet? */ > > > > > > I don't mind adding a comment, but the above is rather self explanatory > > > (one can easily see that it is set in mark_rodata_ro() with a simple > > > search). > > > > > > If a comment is to be added, something a bit more descriptive of the > > > functionality of the variable would be appropriate: > > > > > > /* > > > * Ignore modifying kernel text permissions until the kernel core calls > > > * make_rodata_ro() at system start up. > > > */ > > > > > > I can resend with the comment, or whoever takes this could add it > > > themselves. > > > > Gentle ping: this patch doesn't seem to have landed in upstream > > trees yet. Is any more work required? > > > > It would be nice to have this fix added. Just tested next-20171205 > > on RPi B+, it oopses when the function tracer is enabled during boot. > > next-20171205 plus this patch boots up fine. > > When does it oops? Rather early in the boot process: [0.00] Booting Linux on physical CPU 0x0 [0.00] Linux version 4.15.0-rc2-next-20171205 (hias@camel2) (gcc version 4.9.3 (crosstool-NG crosstool-ng-1.22.0-88-g8460611)) #3 Tue Dec 5 12:35:08 CET 2017 [0.00] CPU: ARMv6-compatible processor [410fb767] revision 7 (ARMv7), cr=00c5387d [0.00] CPU: PIPT / VIPT nonaliasing data cache, VIPT nonaliasing instruction cache [0.00] OF: fdt: Machine model: Raspberry Pi Model B Plus Rev 1.2 [0.00] earlycon: pl11 at MMIO32 0x20201000 (options '') [0.00] bootconsole [pl11] enabled [0.00] Memory policy: Data cache writeback [0.00] cma: Reserved 32 MiB at 0x13c0 [0.00] CPU: All CPU(s) started in SVC mode. [0.00] random: fast init done [0.00] Built 1 zonelists, mobility grouping on. Total pages: 89408 [0.00] Kernel command line: bcm2708_fb.fbwidth=1280 bcm2708_fb.fbheight=1024 bcm2708_fb.fbswap=1 dma.dmachans=0x7f35 bcm2708.boardrev=0x10 bcm2708.serial=0x59ce1e57 bcm2708.uart_clock=4800 bcm2708.disk_led_gpio=47 bcm2708.disk_led_active_low=0 smsc95xx.macaddr=B8:27:EB:CE:1E:57 vc_mem.mem_base=0x1ec0 vc_mem.mem_size=0x2000 root=/dev/mmcblk0p2 rw rootwait elevator=noop earlycon=pl011,mmio32,0x20201000 console=/dev/ttyAMA0,115200 ftrace=function [0.00] Dentry cache hash table entries: 65536 (order: 6, 262144 bytes) [0.00] Inode-cache hash table entries: 32768 (order: 5, 131072 bytes) [0.00] Memory: 311776K/360448K available (7168K kernel code, 561K rwdata, 2212K rodata, 1024K init, 683K bss, 15904K reserved, 32768K cma-reserved) [0.00] Virtual kernel memory layout: [0.00] vector : 0x - 0x1000 ( 4 kB) [0.00] fixmap : 0xffc0 - 0xfff0 (3072 kB) [0.00] vmalloc : 0xd680 - 0xff80 ( 656 MB) [0.00] lowmem : 0xc000 - 0xd600 ( 352 MB) [0.00] modules : 0xbf00 - 0xc000 ( 16 MB) [0.00] .text : 0x(ptrval) - 0x(ptrval) (8160 kB) [0.00] .init : 0x(ptrval) - 0x(ptrval) (1024 kB) [0.00] .data : 0x(ptrval) - 0x(ptrval) ( 562 kB) [0.00].bss : 0x(ptrval) - 0x(ptrval) ( 684 kB) [0.00] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=1, Nodes=1 [0.00] ftrace: allocating 25789 entries in 76 pages [0.00] Starting tracer 'function' [0.00] NR_IRQS: 16, nr_irqs: 16, preallocated irqs: 16 [0.52] sched_clock: 32 bits at 1000kHz, resolution 1000ns, wraps every 2147483647500ns [0.008889] clocksource: timer: mask: 0x max_cycles: 0x, max_idle_ns: 1911260446275 ns [0.018979] bcm2835: system timer (irq = 27) [0.028070] Console: colour dummy device 80x30 [0.033154] Calibrating delay loop..
Re: [PATCH] Arm: mm: ftrace: Only set text back to ro after kernel has been marked ro
On Wed, Aug 23, 2017 at 03:03:51PM -0400, Steven Rostedt wrote:
> On Wed, 23 Aug 2017 11:48:13 -0700
> Kees Cook wrote:
>
> > > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> > > index ad80548..fd75f38 100644
> > > --- a/arch/arm/mm/init.c
> > > +++ b/arch/arm/mm/init.c
> > > @@ -745,19 +745,29 @@ static int __mark_rodata_ro(void *unused)
> > > return 0;
> > > }
> > >
> > > +static int kernel_set_to_readonly;
> >
> > Adding a comment here might be a good idea, something like:
> >
> > /* Has system boot-up reached mark_rodata_ro() yet? */
>
> I don't mind adding a comment, but the above is rather self explanatory
> (one can easily see that it is set in mark_rodata_ro() with a simple
> search).
>
> If a comment is to be added, something a bit more descriptive of the
> functionality of the variable would be appropriate:
>
> /*
> * Ignore modifying kernel text permissions until the kernel core calls
> * make_rodata_ro() at system start up.
> */
>
> I can resend with the comment, or whoever takes this could add it
> themselves.
Gentle ping: this patch doesn't seem to have landed in upstream
trees yet. Is any more work required?
It would be nice to have this fix added. Just tested next-20171205
on RPi B+, it oopses when the function tracer is enabled during boot.
next-20171205 plus this patch boots up fine.
so long,
Hias
>
> -- Steve
>
>
> >
> > Otherwise:
> >
> > Acked-by: Kees Cook
> >
> > > +
> > > void mark_rodata_ro(void)
> > > {
> > > + kernel_set_to_readonly = 1;
> > > +
> > > stop_machine(__mark_rodata_ro, NULL, NULL);
> > > }
> > >
> > > void set_kernel_text_rw(void)
> > > {
> > > + if (!kernel_set_to_readonly)
> > > + return;
> > > +
> > > set_section_perms(ro_perms, ARRAY_SIZE(ro_perms), false,
> > > current->active_mm);
> > > }
> > >
> > > void set_kernel_text_ro(void)
> > > {
> > > + if (!kernel_set_to_readonly)
> > > + return;
> > > +
> > > set_section_perms(ro_perms, ARRAY_SIZE(ro_perms), true,
> > > current->active_mm);
> > > }
> >
> > Does arm64 suffer from a similar condition? (It looks like no, as text
> > patching is done with a fixmap poke.)
> >
> > -Kees
> >
>
Re: [PATCH] media: rc: double keypresses due to timeout expiring to early
Hi Sean!
On Sun, Nov 19, 2017 at 09:57:27PM +, Sean Young wrote:
> I think for now the best solution is to revert to 250ms for all protocols
> (except for cec which needs 550ms), and reconsider for another kernel.
Thanks, this sounds like a good idea!
> >>From 2f1135f3f9873778ca5c013d1118710152840cb2 Mon Sep 17 00:00:00 2001
> From: Sean Young
> Date: Sun, 19 Nov 2017 21:11:17 +
> Subject: [PATCH] media: rc: partial revert of "media: rc: per-protocol repeat
> period"
>
> Since commit d57ea877af38 ("media: rc: per-protocol repeat period"), most
> IR protocols have a lower keyup timeout. This causes problems on the
> ite-cir, which has default IR timeout of 200ms.
>
> Since the IR decoders read the trailing space, with a IR timeout of 200ms,
> the last keydown will have at least a delay of 200ms. This is more than
> the protocol timeout of e.g. rc-6 (which is 164ms). As a result the last
> IR will be interpreted as a new keydown event, and we get two keypresses.
>
> Revert the protocol timeout to 250ms, except for cec which needs a timeout
> of 550ms.
>
> Fixes: d57ea877af38 ("media: rc: per-protocol repeat period")
> Cc: # 4.14
> Signed-off-by: Sean Young
Tested-by: Matthias Reichl
I tested this locally with gpio-ir configured to 200ms timeout and
we also received feedback from 2 users that this change fixed the
issue with the ite-cir receiver.
https://forum.kodi.tv/showthread.php?tid=298462&pid=2670637#pid2670637
Thanks a lot for fixing this so quickly!
so long,
Hias
> ---
> drivers/media/rc/rc-main.c | 32
> 1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 17950e29d4e3..5057b2ba0c10 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -39,41 +39,41 @@ static const struct {
> [RC_PROTO_UNKNOWN] = { .name = "unknown", .repeat_period = 250 },
> [RC_PROTO_OTHER] = { .name = "other", .repeat_period = 250 },
> [RC_PROTO_RC5] = { .name = "rc-5",
> - .scancode_bits = 0x1f7f, .repeat_period = 164 },
> + .scancode_bits = 0x1f7f, .repeat_period = 250 },
> [RC_PROTO_RC5X_20] = { .name = "rc-5x-20",
> - .scancode_bits = 0x1f7f3f, .repeat_period = 164 },
> + .scancode_bits = 0x1f7f3f, .repeat_period = 250 },
> [RC_PROTO_RC5_SZ] = { .name = "rc-5-sz",
> - .scancode_bits = 0x2fff, .repeat_period = 164 },
> + .scancode_bits = 0x2fff, .repeat_period = 250 },
> [RC_PROTO_JVC] = { .name = "jvc",
> .scancode_bits = 0x, .repeat_period = 250 },
> [RC_PROTO_SONY12] = { .name = "sony-12",
> - .scancode_bits = 0x1f007f, .repeat_period = 100 },
> + .scancode_bits = 0x1f007f, .repeat_period = 250 },
> [RC_PROTO_SONY15] = { .name = "sony-15",
> - .scancode_bits = 0xff007f, .repeat_period = 100 },
> + .scancode_bits = 0xff007f, .repeat_period = 250 },
> [RC_PROTO_SONY20] = { .name = "sony-20",
> - .scancode_bits = 0x1fff7f, .repeat_period = 100 },
> + .scancode_bits = 0x1fff7f, .repeat_period = 250 },
> [RC_PROTO_NEC] = { .name = "nec",
> - .scancode_bits = 0x, .repeat_period = 160 },
> + .scancode_bits = 0x, .repeat_period = 250 },
> [RC_PROTO_NECX] = { .name = "nec-x",
> - .scancode_bits = 0xff, .repeat_period = 160 },
> + .scancode_bits = 0xff, .repeat_period = 250 },
> [RC_PROTO_NEC32] = { .name = "nec-32",
> - .scancode_bits = 0x, .repeat_period = 160 },
> + .scancode_bits = 0x, .repeat_period = 250 },
> [RC_PROTO_SANYO] = { .name = "sanyo",
> .scancode_bits = 0x1f, .repeat_period = 250 },
> [RC_PROTO_MCIR2_KBD] = { .name = "mcir2-kbd",
> - .scancode_bits = 0x, .repeat_period = 150 },
> + .scancode_bits = 0x, .repeat_period = 250 },
> [RC_PROTO_MCIR2_MSE] = { .name = "mcir2-mse",
> - .scancode_bits = 0x1f, .repeat_period = 150 },
> + .scancode_bits = 0x1f, .repeat_period = 250 },
> [RC_PROTO_RC6_0] = { .name = "rc-6-0",
> - .scancode_bits = 0x, .repeat_period = 164 },
> + .scancode_bits = 0x, .repeat_period = 250 },
> [RC_PROTO_RC6_6A_20] = { .name = "rc-6-6a-20",
> - .scancode_bits = 0xf, .repeat_period = 164 },
> +
Re: [PATCH] media: rc: double keypresses due to timeout expiring to early
On Fri, Nov 17, 2017 at 03:52:50PM +0100, Matthias Reichl wrote:
> Hi Sean!
>
> On Thu, Nov 16, 2017 at 09:54:51PM +, Sean Young wrote:
> > Since commit d57ea877af38 ("media: rc: per-protocol repeat period"),
> > double keypresses are reported on the ite-cir driver. This is due
> > two factors: that commit reduced the timeout used for some protocols
> > (it became protocol dependant) and the high default IR timeout used
> > by the ite-cir driver.
> >
> > Some of the IR decoders wait for a trailing space, as that is
> > the only way to know if the bit stream has ended (e.g. rc-6 can be
> > 16, 20 or 32 bits). The longer the IR timeout, the longer it will take
> > to receive the trailing space, delaying decoding and pushing it past the
> > keyup timeout.
> >
> > So, add the IR timeout to the keyup timeout.
>
> Thanks a lot for the patch, I've asked the people with ite-cir
> receivers to test it.
Just got the first test results from ite-cir + rc-6 remote back,
events were the same as I was seeing with gpio-ir-recv with 200ms
timeout - key repeat event from input layer.
Kernel 4.14 + your patch:
Event: time 1510938801.797335, type 4 (EV_MSC), code 4 (MSC_SCAN), value
800f041f
Event: time 1510938801.797335, type 1 (EV_KEY), code 108 (KEY_DOWN), value 1
Event: time 1510938801.797335, -- SYN_REPORT
Event: time 1510938802.034331, type 4 (EV_MSC), code 4 (MSC_SCAN), value
800f041f
Event: time 1510938802.034331, -- SYN_REPORT
Event: time 1510938802.301333, type 1 (EV_KEY), code 108 (KEY_DOWN), value 2
Event: time 1510938802.301333, -- SYN_REPORT
Event: time 1510938802.408003, type 1 (EV_KEY), code 108 (KEY_DOWN), value 0
Event: time 1510938802.408003, -- SYN_REPORT
Kernel 4.13.2:
Event: time 1510938637.791450, type 4 (EV_MSC), code 4 (MSC_SCAN), value
800f041f
Event: time 1510938637.791450, type 1 (EV_KEY), code 108 (KEY_DOWN), value 1
Event: time 1510938637.791450, -- SYN_REPORT
Event: time 1510938638.028301, type 4 (EV_MSC), code 4 (MSC_SCAN), value
800f041f
Event: time 1510938638.028301, -- SYN_REPORT
Event: time 1510938638.292323, type 1 (EV_KEY), code 108 (KEY_DOWN), value 0
Event: time 1510938638.292323, -- SYN_REPORT
so long,
Hias
>
> In the meanwhile I ran some tests with gpio-ir-recv and timeout
> set to 200ms with a rc-5 remote (that's as close to the original
> setup as I can test right now).
>
> While the patch fixes the additional key down/up event on longer
> presses, I still get a repeated key event on a short button
> press - which makes it hard to do a single click with the
> remote.
>
> Test on kernel 4.14 with your patch:
> 1510927844.292126: event type EV_MSC(0x04): scancode = 0x1015
> 1510927844.292126: event type EV_KEY(0x01) key_down: KEY_ENTER(0x001c)
> 1510927844.292126: event type EV_SYN(0x00).
> 1510927844.498773: event type EV_MSC(0x04): scancode = 0x1015
> 1510927844.498773: event type EV_SYN(0x00).
> 1510927844.795410: event type EV_KEY(0x01) key_down: KEY_ENTER(0x001c)
> 1510927844.795410: event type EV_SYN(0x00).
> 1510927844.875412: event type EV_KEY(0x01) key_up: KEY_ENTER(0x001c)
> 1510927844.875412: event type EV_SYN(0x00).
>
> Same signal received on kernel 4.9:
> 1510927844.280350: event type EV_MSC(0x04): scancode = 0x1015
> 1510927844.280350: event type EV_KEY(0x01) key_down: KEY_OK(0x0160)
> 1510927844.280350: event type EV_SYN(0x00).
> 1510927844.506477: event type EV_MSC(0x04): scancode = 0x1015
> 1510927844.506477: event type EV_SYN(0x00).
> 1510927844.763111: event type EV_KEY(0x01) key_up: KEY_OK(0x0160)
> 1510927844.763111: event type EV_SYN(0x00).
>
> If I understand it correctly it's the input layer repeat (500ms delay)
> kicking in, because time between initial scancode and timeout is
> now signal time + 200ms + 164ms + 200ms (about 570-580ms).
> On older kernels this was signal time + 200ms + 250ms, so typically
> just below the 500ms default repeat delay.
>
> I'm still trying to wrap my head around the timeout code in the
> rc layer. One problem seems to be that we apply the rather large
> timeout twice. Maybe detecting scancodes generated via timeout
> (sth like timestamp - last_timestamp > protocol_repeat_period)
> and in that case immediately signalling keyup could help? Could well
> be that I'm missing somehting important and this is a bad idea.
> I guess I'd better let you figure something out :)
>
> so long,
>
> Hias
>
> >
> > Cc: # 4.14
> > Reported-by: Matthias Reichl
> > Signed-off-by: Sean Young
> > ---
> > drivers/media/rc/rc-main.
Re: [PATCH] media: rc: double keypresses due to timeout expiring to early
Hi Sean!
On Thu, Nov 16, 2017 at 09:54:51PM +, Sean Young wrote:
> Since commit d57ea877af38 ("media: rc: per-protocol repeat period"),
> double keypresses are reported on the ite-cir driver. This is due
> two factors: that commit reduced the timeout used for some protocols
> (it became protocol dependant) and the high default IR timeout used
> by the ite-cir driver.
>
> Some of the IR decoders wait for a trailing space, as that is
> the only way to know if the bit stream has ended (e.g. rc-6 can be
> 16, 20 or 32 bits). The longer the IR timeout, the longer it will take
> to receive the trailing space, delaying decoding and pushing it past the
> keyup timeout.
>
> So, add the IR timeout to the keyup timeout.
Thanks a lot for the patch, I've asked the people with ite-cir
receivers to test it.
In the meanwhile I ran some tests with gpio-ir-recv and timeout
set to 200ms with a rc-5 remote (that's as close to the original
setup as I can test right now).
While the patch fixes the additional key down/up event on longer
presses, I still get a repeated key event on a short button
press - which makes it hard to do a single click with the
remote.
Test on kernel 4.14 with your patch:
1510927844.292126: event type EV_MSC(0x04): scancode = 0x1015
1510927844.292126: event type EV_KEY(0x01) key_down: KEY_ENTER(0x001c)
1510927844.292126: event type EV_SYN(0x00).
1510927844.498773: event type EV_MSC(0x04): scancode = 0x1015
1510927844.498773: event type EV_SYN(0x00).
1510927844.795410: event type EV_KEY(0x01) key_down: KEY_ENTER(0x001c)
1510927844.795410: event type EV_SYN(0x00).
1510927844.875412: event type EV_KEY(0x01) key_up: KEY_ENTER(0x001c)
1510927844.875412: event type EV_SYN(0x00).
Same signal received on kernel 4.9:
1510927844.280350: event type EV_MSC(0x04): scancode = 0x1015
1510927844.280350: event type EV_KEY(0x01) key_down: KEY_OK(0x0160)
1510927844.280350: event type EV_SYN(0x00).
1510927844.506477: event type EV_MSC(0x04): scancode = 0x1015
1510927844.506477: event type EV_SYN(0x00).
1510927844.763111: event type EV_KEY(0x01) key_up: KEY_OK(0x0160)
1510927844.763111: event type EV_SYN(0x00).
If I understand it correctly it's the input layer repeat (500ms delay)
kicking in, because time between initial scancode and timeout is
now signal time + 200ms + 164ms + 200ms (about 570-580ms).
On older kernels this was signal time + 200ms + 250ms, so typically
just below the 500ms default repeat delay.
I'm still trying to wrap my head around the timeout code in the
rc layer. One problem seems to be that we apply the rather large
timeout twice. Maybe detecting scancodes generated via timeout
(sth like timestamp - last_timestamp > protocol_repeat_period)
and in that case immediately signalling keyup could help? Could well
be that I'm missing somehting important and this is a bad idea.
I guess I'd better let you figure something out :)
so long,
Hias
>
> Cc: # 4.14
> Reported-by: Matthias Reichl
> Signed-off-by: Sean Young
> ---
> drivers/media/rc/rc-main.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 17950e29d4e3..fae721534517 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -672,7 +672,8 @@ void rc_repeat(struct rc_dev *dev)
> input_event(dev->input_dev, EV_MSC, MSC_SCAN, dev->last_scancode);
> input_sync(dev->input_dev);
>
> - dev->keyup_jiffies = jiffies + msecs_to_jiffies(timeout);
> + dev->keyup_jiffies = jiffies + msecs_to_jiffies(timeout) +
> + nsecs_to_jiffies(dev->timeout);
> mod_timer(&dev->timer_keyup, dev->keyup_jiffies);
>
> out:
> @@ -744,7 +745,8 @@ void rc_keydown(struct rc_dev *dev, enum rc_proto
> protocol, u32 scancode,
>
> if (dev->keypressed) {
> dev->keyup_jiffies = jiffies +
> - msecs_to_jiffies(protocols[protocol].repeat_period);
> + msecs_to_jiffies(protocols[protocol].repeat_period) +
> + nsecs_to_jiffies(dev->timeout);
> mod_timer(&dev->timer_keyup, dev->keyup_jiffies);
> }
> spin_unlock_irqrestore(&dev->keylock, flags);
> --
> 2.14.3
>
4.14 regression from commit d57ea877af38 media: rc: per-protocol repeat period
The following commit introduced a regression commit d57ea877af38057b0ef31758cf3b99765dc33695 Author: Sean Young Date: Wed Aug 9 13:19:16 2017 -0400 media: rc: per-protocol repeat period CEC needs a keypress timeout of 550ms, which is too high for the IR protocols. Also fill in known repeat times, with 50ms error margin. Also, combine all protocol data into one structure. We received a report that an RC6 MCE remote used with the ite-cir produces "double events" on short button presses: https://forum.kodi.tv/showthread.php?tid=298462&pid=2667855#pid2667855 Looking at the ir-keytable -t output an additional key event is also generated after longer button presses: # ir-keytable -t Testing events. Please, press CTRL-C to abort. 1510680591.697657: event type EV_MSC(0x04): scancode = 0x800f041f 1510680591.697657: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c) 1510680591.697657: event type EV_SYN(0x00). 1510680591.867355: event type EV_KEY(0x01) key_up: KEY_DOWN(0x006c) 1510680591.867355: event type EV_SYN(0x00). 1510680591.935026: event type EV_MSC(0x04): scancode = 0x800f041f 1510680591.935026: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c) 1510680591.935026: event type EV_SYN(0x00). 1510680592.104100: event type EV_KEY(0x01) key_up: KEY_DOWN(0x006c) 1510680592.104100: event type EV_SYN(0x00). 1510680597.714055: event type EV_MSC(0x04): scancode = 0x800f0405 1510680597.714055: event type EV_KEY(0x01) key_down: KEY_NUMERIC_5(0x0205) 1510680597.714055: event type EV_SYN(0x00). 1510680597.819939: event type EV_MSC(0x04): scancode = 0x800f0405 1510680597.819939: event type EV_SYN(0x00). 1510680597.925614: event type EV_MSC(0x04): scancode = 0x800f0405 1510680597.925614: event type EV_SYN(0x00). 1510680598.032422: event type EV_MSC(0x04): scancode = 0x800f0405 1510680598.032422: event type EV_SYN(0x00). ... 1510680598.562114: event type EV_MSC(0x04): scancode = 0x800f0405 1510680598.562114: event type EV_SYN(0x00). 1510680598.630641: event type EV_KEY(0x01) key_down: KEY_NUMERIC_5(0x0205) 1510680598.630641: event type EV_SYN(0x00). 1510680598.667906: event type EV_MSC(0x04): scancode = 0x800f0405 1510680598.667906: event type EV_SYN(0x00). 1510680598.760760: event type EV_KEY(0x01) key_down: KEY_NUMERIC_5(0x0205) 1510680598.760760: event type EV_SYN(0x00). 1510680598.837412: event type EV_KEY(0x01) key_up: KEY_NUMERIC_5(0x0205) 1510680598.837412: event type EV_SYN(0x00). 1510680598.905003: event type EV_MSC(0x04): scancode = 0x800f0405 1510680598.905003: event type EV_KEY(0x01) key_down: KEY_NUMERIC_5(0x0205) 1510680598.905003: event type EV_SYN(0x00). 1510680599.074092: event type EV_KEY(0x01) key_up: KEY_NUMERIC_5(0x0205) 1510680599.074092: event type EV_SYN(0x00). Looking at the timestamps of the scancode events it seems that signals are received every ~106ms but the last signal seems to be received 237ms after the last-but-one - which is then interpreted as a new key press cycle as the delay is longer than the 164ms "repeat_period" setting of the RC6 protocol (before that commit 250ms was used). This 237ms delay seems to be coming from the 200ms timeout value of the ite-cir driver (237ms is in the ballpark of ~30ms rc6 signal time plus 200ms timeout). The original author hasn't reported back yet but others confirmed that changing the timeout to 100ms (minimum idle timeout value of ite-cir) using "ir-ctl -t 10" fixes the issue. I could locally reproduce this with gpio-ir-recv (which uses the default 125ms timeout) and the sony protocol (repeat_period = 100ms): 1510838115.272021: event type EV_MSC(0x04): scancode = 0x110001 1510838115.272021: event type EV_KEY(0x01) key_down: KEY_2(0x0003) 1510838115.272021: event type EV_SYN(0x00). 1510838115.322014: event type EV_MSC(0x04): scancode = 0x110001 1510838115.322014: event type EV_SYN(0x00). 1510838115.362003: event type EV_MSC(0x04): scancode = 0x110001 1510838115.362003: event type EV_SYN(0x00). 1510838115.412002: event type EV_MSC(0x04): scancode = 0x110001 1510838115.412002: event type EV_SYN(0x00). 1510838115.521973: event type EV_KEY(0x01) key_up: KEY_2(0x0003) 1510838115.521973: event type EV_SYN(0x00). 1510838115.532007: event type EV_MSC(0x04): scancode = 0x110001 1510838115.532007: event type EV_KEY(0x01) key_down: KEY_2(0x0003) 1510838115.532007: event type EV_SYN(0x00). 1510838115.641972: event type EV_KEY(0x01) key_up: KEY_2(0x0003) 1510838115.641972: event type EV_SYN(0x00). Reducing the timeout to 20ms removes the addional key_down/up event. Another test with a rc-5 remote on gpio-ir-recv worked fine at the default 125ms timeout but with 200ms as on the ite-cir I again got the additional key event. so long, Hias
Re: [PATCH] Arm: mm: ftrace: Only set text back to ro after kernel has been marked ro
On Wed, Aug 23, 2017 at 01:58:36PM -0400, Steven Rostedt wrote:
>
> ftrace needs to modify the kernel text in order to enable function tracing.
> For security reasons, the kernel text is marked to read-only (ro) at the end
> of system bootup. When enabling function tracing after that, ftrace calls
> arch specific code that needs to enable the modification of kernel text
> while ftrace does the update, and reset it back again when finished.
>
> The issue arises when function tracing is enabled during system bootup. The
> text hasn't been marked as read-only yet, but the same code to modify the
> kernel is executed, and when it is finished, it will cause the kernel to
> become read-only. This causes issues for other init code that requires
> modification of kernel text during system bootup. This appears to cause
> issue with Raspberry Pi 2.
>
> By implementing the feature that is used in x86 to deal with this issue, it
> fixes the problem. The solution is simple. Have a variable
> (kernel_set_to_readonly) get set when the system finished boot and marks the
> kernel to readonly. If that variable is not set, both
> kernel_set_to_readonly() and kernel_set_to_rw() return without doing any
> modifications. Those functions are used by ftrace to change the permissions
> of the kernel text. By not doing anything, ftrace will not mess with the
> permissions when it is enabled at system bootup.
>
> Link: http://lkml.kernel.org/r/20170821153402.7so2u364htvt6...@camel2.lan
> Link: https://github.com/raspberrypi/linux/issues/2166#issuecomment-323355145
> Reported-by: Matthias Reichl
> Cc: Russell King
> Cc: Kees Cook
> Cc: Eric Anholt
> Cc: Stefan Wahren
> Cc: Phil Elwell
> Cc: linux-rpi-ker...@lists.infradead.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: sta...@vger.kernel.org
> Fixes: 80d6b0c2ee ("ARM: mm: allow text and rodata sections to be read-only")
> Signed-off-by: Steven Rostedt (VMware)
Tested-by: Matthias Reichl
Thanks, again, for resolving the issue so quickly!
so long,
Hias
> ---
> arch/arm/mm/init.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index ad80548..fd75f38 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -745,19 +745,29 @@ static int __mark_rodata_ro(void *unused)
> return 0;
> }
>
> +static int kernel_set_to_readonly;
> +
> void mark_rodata_ro(void)
> {
> + kernel_set_to_readonly = 1;
> +
> stop_machine(__mark_rodata_ro, NULL, NULL);
> }
>
> void set_kernel_text_rw(void)
> {
> + if (!kernel_set_to_readonly)
> + return;
> +
> set_section_perms(ro_perms, ARRAY_SIZE(ro_perms), false,
> current->active_mm);
> }
>
> void set_kernel_text_ro(void)
> {
> + if (!kernel_set_to_readonly)
> + return;
> +
> set_section_perms(ro_perms, ARRAY_SIZE(ro_perms), true,
> current->active_mm);
> }
> --
> 2.9.5
>
[RESEND PATCH] dmaengine: bcm2835: Fix cyclic DMA period splitting
The code responsible for splitting periods into chunks that
can be handled by the DMA controller missed to update total_len,
the number of bytes processed in the current period, when there
are more chunks to follow.
Therefore total_len was stuck at 0 and the code didn't work at all.
This resulted in a wrong control block layout and audio issues because
the cyclic DMA callback wasn't executing on period boundaries.
Fix this by adding the missing total_len update.
Signed-off-by: Matthias Reichl
Signed-off-by: Martin Sperl
Tested-by: Clive Messer
Reviewed-by: Eric Anholt
---
drivers/dma/bcm2835-dma.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
index 80d35f7..599c218 100644
--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -253,8 +253,11 @@ static void bcm2835_dma_create_cb_set_length(
*/
/* have we filled in period_length yet? */
- if (*total_len + control_block->length < period_len)
+ if (*total_len + control_block->length < period_len) {
+ /* update number of bytes in this period so far */
+ *total_len += control_block->length;
return;
+ }
/* calculate the length that remains to reach period_length */
control_block->length = period_len - *total_len;
--
2.1.4
Re: [PATCH 2/2] dmaengine: bcm2835: Avoid splitting periods into very small chunks
On Mon, Jun 13, 2016 at 10:06:49PM -0700, Eric Anholt wrote:
> Matthias Reichl writes:
>
> > The current cyclic DMA period splitting implementation can generate
> > very small chunks at the end of each period. For example a 65536 byte
> > period will be split into a 65532 byte chunk and a 4 byte chunk on
> > the "lite" DMA channels.
> >
> > This increases pressure on the RAM controller as the DMA controller
> > needs to fetch two control blocks from RAM in quick succession and
> > could potentially cause latency issues if the RAM is tied up by other
> > devices.
> >
> > We can easily avoid these situations by distributing the remaining
> > length evenly between the last-but-one and the last chunk, making
> > sure that split chunks will be at least half the maximum length the
> > DMA controller can handle.
> >
> > This patch checks if the last chunk would be less than half of
> > the maximum DMA length and if yes distributes the max len+4...max_len*1.5
> > bytes evenly between the last 2 chunks. This results in chunk sizes
> > between max_len/2 and max_len*0.75 bytes.
> >
> > Signed-off-by: Matthias Reichl
> > Signed-off-by: Martin Sperl
> > Tested-by: Clive Messer
> > ---
> > drivers/dma/bcm2835-dma.c | 14 ++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
> > index 344bcf92..36b998d 100644
> > --- a/drivers/dma/bcm2835-dma.c
> > +++ b/drivers/dma/bcm2835-dma.c
> > @@ -252,6 +252,20 @@ static void bcm2835_dma_create_cb_set_length(
> >
> > /* have we filled in period_length yet? */
> > if (*total_len + control_block->length < period_len) {
> > + /*
> > +* If the next control block is the last in the period
> > +* and it's length would be less than half of max_len
> > +* change it so that both control blocks are (almost)
> > +* equally long. This avoids generating very short
> > +* control blocks (worst case would be 4 bytes) which
> > +* might be problematic. We also have to make sure the
> > +* new length is a multiple of 4 bytes.
> > +*/
> > + if (*total_len + control_block->length + max_len / 2 >
> > + period_len) {
> > + control_block->length =
> > + DIV_ROUND_UP(period_len - *total_len, 8) * 4;
> > + }
> > /* update number of bytes in this period so far */
> > *total_len += control_block->length;
> > return;
>
> It seems to me like this would all be a lot simpler if we always split
> the last 2 control blocks evenly (other than 4-byte rounding):
Agreed and thanks a lot for the feedback!
I'll do it that way and then send out a v2.
> u32 period_remaining = period_len - *total_len;
>
> /* Early exit if we aren't finishing this period */
> if (period_remaining >= max_len) {
This has to be > max_len, but the rest seems fine. We want to split
if we have more than max_len but less than max_len*2 bytes.
> /*
>* Split the length between the last 2 CBs, to help hide the
>* latency of fetching the CBs.
>*/
> if (period_remaining < max_len * 2) {
> control_block->length =
> DIV_ROUND_UP(period_remaining, 8) * 4;
> }
> /* update number of bytes in this period so far */
> *total_len += control_block->length;
> }
>
> I'm about to go semi-AFK for a couple weeks. If there's a good reason
> to only do this when the last block is very short, I'm fine with:
>
> Acked-by: Eric Anholt
[PATCH 0/2] dmaengine: bcm2835: Cyclic DMA fixes
In downstream Raspberry Pi kernel we noticed that audio didn't work as expected, we got stuttering and overruns/underruns. Here's the link to the original discussion on GitHub: https://github.com/raspberrypi/linux/issues/1517 This issue is caused by a small bug in the period-splitting-code and fixed by the first patch. The second patch, avoiding very small chunks, is mainly a precaution. While small chunks are not known to have caused any problems so far they have the potentical to cause very hard to track down issues. So better avoid such situations in the first place. Matthias Reichl (2): dmaengine: bcm2835: Fix cyclic DMA period splitting dmaengine: bcm2835: Avoid splitting periods into very small chunks drivers/dma/bcm2835-dma.c | 19 ++- 1 file changed, 18 insertions(+), 1 deletion(-) -- 2.1.4
[PATCH 2/2] dmaengine: bcm2835: Avoid splitting periods into very small chunks
The current cyclic DMA period splitting implementation can generate
very small chunks at the end of each period. For example a 65536 byte
period will be split into a 65532 byte chunk and a 4 byte chunk on
the "lite" DMA channels.
This increases pressure on the RAM controller as the DMA controller
needs to fetch two control blocks from RAM in quick succession and
could potentially cause latency issues if the RAM is tied up by other
devices.
We can easily avoid these situations by distributing the remaining
length evenly between the last-but-one and the last chunk, making
sure that split chunks will be at least half the maximum length the
DMA controller can handle.
This patch checks if the last chunk would be less than half of
the maximum DMA length and if yes distributes the max len+4...max_len*1.5
bytes evenly between the last 2 chunks. This results in chunk sizes
between max_len/2 and max_len*0.75 bytes.
Signed-off-by: Matthias Reichl
Signed-off-by: Martin Sperl
Tested-by: Clive Messer
---
drivers/dma/bcm2835-dma.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
index 344bcf92..36b998d 100644
--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -252,6 +252,20 @@ static void bcm2835_dma_create_cb_set_length(
/* have we filled in period_length yet? */
if (*total_len + control_block->length < period_len) {
+ /*
+* If the next control block is the last in the period
+* and it's length would be less than half of max_len
+* change it so that both control blocks are (almost)
+* equally long. This avoids generating very short
+* control blocks (worst case would be 4 bytes) which
+* might be problematic. We also have to make sure the
+* new length is a multiple of 4 bytes.
+*/
+ if (*total_len + control_block->length + max_len / 2 >
+ period_len) {
+ control_block->length =
+ DIV_ROUND_UP(period_len - *total_len, 8) * 4;
+ }
/* update number of bytes in this period so far */
*total_len += control_block->length;
return;
--
2.1.4
[PATCH 1/2] dmaengine: bcm2835: Fix cyclic DMA period splitting
The code responsible for splitting periods into chunks that
can be handled by the DMA controller missed to update total_len,
the number of bytes processed in the current period, when there
are more chunks to follow.
Therefore total_len was stuck at 0 and the code didn't work at all.
This resulted in a wrong control block layout and audio issues because
the cyclic DMA callback wasn't executing on period boundaries.
Fix this by adding the missing total_len update.
Signed-off-by: Matthias Reichl
Signed-off-by: Martin Sperl
Tested-by: Clive Messer
---
drivers/dma/bcm2835-dma.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
index 6149b27..344bcf92 100644
--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -251,8 +251,11 @@ static void bcm2835_dma_create_cb_set_length(
*/
/* have we filled in period_length yet? */
- if (*total_len + control_block->length < period_len)
+ if (*total_len + control_block->length < period_len) {
+ /* update number of bytes in this period so far */
+ *total_len += control_block->length;
return;
+ }
/* calculate the length that remains to reach period_length */
control_block->length = period_len - *total_len;
--
2.1.4
[PATCH 2/2] ASoC: bcm2835: Add S16_LE support via packed DMA transfers
The bcm2835-i2s driver already has support for the S16_LE format but that format hasn't been made available because dmaengine_pcm didn't support packed data transfers. bcm2835-i2s needs 16-bit left+right channel data to be packed into a 32-bit word, the FIFO register is 32-bit only and doesn't support 16-bit access. Now that dmaengine_pcm supports packed transfers the format can be made available by setting the SND_DMAENGINE_PCM_DAI_FLAG_PACK flag. No further configuration is necessary: - snd_dmaengine_dai_dma_data.addr_width is already set to DMA_SLAVE_BUSWIDTH_4_BYTES to force 32-bit DMA transfers - dmaengine_pcm will pick up the S16_LE format from the DAI configuration and make it available since it's no longer masked out due to the PACK flag. - there are no further corner cases to catch in hw_params, since the channel count is fixed at 2 we always have two 16-bit stereo samples that can be transferred via 32-bit DMA Signed-off-by: Matthias Reichl --- sound/soc/bcm/bcm2835-i2s.c | 9 + 1 file changed, 9 insertions(+) diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c index 1c1f221..40ba01d 100644 --- a/sound/soc/bcm/bcm2835-i2s.c +++ b/sound/soc/bcm/bcm2835-i2s.c @@ -678,6 +678,15 @@ static int bcm2835_i2s_probe(struct platform_device *pdev) dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].maxburst = 2; dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].maxburst = 2; + /* +* Set the PACK flag to enable S16_LE support (2 S16_LE values +* packed into 32-bit transfers). +*/ + dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].flags = + SND_DMAENGINE_PCM_DAI_FLAG_PACK; + dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].flags = + SND_DMAENGINE_PCM_DAI_FLAG_PACK; + /* BCLK ratio - use default */ dev->bclk_ratio = 0; -- 2.1.4
[PATCH 1/2] ASoC: dmaengine_pcm: Add support for packed transfers
dmaengine_pcm currently only supports setups where FIFO reads/writes
correspond to exactly one sample, eg 16-bit sample data is transferred
via 16-bit FIFO accesses, 32-bit data via 32-bit accesses.
This patch adds support for setups with fixed width FIFOs where
multiple samples are packed into a larger word.
For example setups with a 32-bit wide FIFO register that expect
16-bit sample transfers to be done with the left+right sample data
packed into a 32-bit word.
Support for packed transfers is controlled via the
SND_DMAENGINE_PCM_DAI_FLAG_PACK flag in snd_dmaengine_dai_dma_data.flags
If this flag is set dmaengine_pcm doesn't put any restriction on the
supported formats and sets the DMA transfer width to undefined.
This means control over the constraints is now transferred to the DAI
driver and it's responsible to provide proper configuration and
check for possible corner cases that aren't handled by the ALSA core.
Signed-off-by: Matthias Reichl
---
include/sound/dmaengine_pcm.h | 12
sound/core/pcm_dmaengine.c| 11 +--
sound/soc/soc-generic-dmaengine-pcm.c | 57 +--
3 files changed, 55 insertions(+), 25 deletions(-)
diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h
index f86ef5e..67be244 100644
--- a/include/sound/dmaengine_pcm.h
+++ b/include/sound/dmaengine_pcm.h
@@ -51,6 +51,16 @@ struct dma_chan
*snd_dmaengine_pcm_request_channel(dma_filter_fn filter_fn,
void *filter_data);
struct dma_chan *snd_dmaengine_pcm_get_chan(struct snd_pcm_substream
*substream);
+/*
+ * The DAI supports packed transfers, eg 2 16-bit samples in a 32-bit word.
+ * If this flag is set the dmaengine driver won't put any restriction on
+ * the supported sample formats and set the DMA transfer size to undefined.
+ * The DAI driver is responsible to disable any unsupported formats in it's
+ * configuration and catch corner cases that are not already handled in
+ * the ALSA core.
+ */
+#define SND_DMAENGINE_PCM_DAI_FLAG_PACK BIT(0)
+
/**
* struct snd_dmaengine_dai_dma_data - DAI DMA configuration data
* @addr: Address of the DAI data source or destination register.
@@ -63,6 +73,7 @@ struct dma_chan *snd_dmaengine_pcm_get_chan(struct
snd_pcm_substream *substream)
* requesting the DMA channel.
* @chan_name: Custom channel name to use when requesting DMA channel.
* @fifo_size: FIFO size of the DAI controller in bytes
+ * @flags: PCM_DAI flags, only SND_DMAENGINE_PCM_DAI_FLAG_PACK for now
*/
struct snd_dmaengine_dai_dma_data {
dma_addr_t addr;
@@ -72,6 +83,7 @@ struct snd_dmaengine_dai_dma_data {
void *filter_data;
const char *chan_name;
unsigned int fifo_size;
+ unsigned int flags;
};
void snd_dmaengine_pcm_set_config_from_dai_data(
diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c
index 697c166..8eb58c7 100644
--- a/sound/core/pcm_dmaengine.c
+++ b/sound/core/pcm_dmaengine.c
@@ -106,8 +106,9 @@ EXPORT_SYMBOL_GPL(snd_hwparams_to_dma_slave_config);
* direction of the substream. If the substream is a playback stream the dst
* fields will be initialized, if it is a capture stream the src fields will be
* initialized. The {dst,src}_addr_width field will only be initialized if the
- * addr_width field of the DAI DMA data struct is not equal to
- * DMA_SLAVE_BUSWIDTH_UNDEFINED.
+ * SND_DMAENGINE_PCM_DAI_FLAG_PACK flag is set or if the addr_width field of
+ * the DAI DMA data struct is not equal to DMA_SLAVE_BUSWIDTH_UNDEFINED. If
+ * both conditions are met the latter takes priority.
*/
void snd_dmaengine_pcm_set_config_from_dai_data(
const struct snd_pcm_substream *substream,
@@ -117,11 +118,17 @@ void snd_dmaengine_pcm_set_config_from_dai_data(
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
slave_config->dst_addr = dma_data->addr;
slave_config->dst_maxburst = dma_data->maxburst;
+ if (dma_data->flags & SND_DMAENGINE_PCM_DAI_FLAG_PACK)
+ slave_config->dst_addr_width =
+ DMA_SLAVE_BUSWIDTH_UNDEFINED;
if (dma_data->addr_width != DMA_SLAVE_BUSWIDTH_UNDEFINED)
slave_config->dst_addr_width = dma_data->addr_width;
} else {
slave_config->src_addr = dma_data->addr;
slave_config->src_maxburst = dma_data->maxburst;
+ if (dma_data->flags & SND_DMAENGINE_PCM_DAI_FLAG_PACK)
+ slave_config->src_addr_width =
+ DMA_SLAVE_BUSWIDTH_UNDEFINED;
if (dma_data->addr_width != DMA_SLAVE_BUSWIDTH_UNDEFINED)
slave_config->src_addr_width = dma_data->addr_width;
}
diff --git a/sound/soc/soc-generic-dmaengine-pcm.c
b/sound/soc/soc-generic-dmaengine-pcm.c
index 6fd1906..6cef39
