Re: linux-next: build warning after merge of the fbdev tree
On Tue, Apr 02, 2019 at 09:30:07AM +1100, Stephen Rothwell wrote:
> Hi Bartlomiej,
>
> After merging the fbdev tree, today's linux-next build (x86_64
> allmodconfig) produced this warning:
>
> drivers/video/fbdev/sm712fb.c: In function 'smtc_blank':
> drivers/video/fbdev/sm712fb.c:900:4: warning: this statement may fall through
> [-Wimplicit-fallthrough=]
> smtc_seqw(0x6b, 0x02);
> ^
> drivers/video/fbdev/sm712fb.c:901:3: note: here
>case 0x720:
>^~~~
>
> Introduced by commit
>
> f627caf55b8e ("fbdev: sm712fb: fix crashes and garbled display during DPMS
> modesetting")
>
Nice catch! Thanks!
This bug was introduced by me while attempting to fix another issue, a result of
my copy-paste error. Since it only reprograms the clock to a different
frequency,
it's only a benign issue without visible side-effect, so it also evaded Sudip
Mukherjee's code review and regression tests.
But what's I'm more concerned here is the failure of scripts/checkpatch.pl. I
thought ./checkpatch.pl should have caught it, but for some reasons it cannot
detect this one.
$ ./scripts/checkpatch.pl
0001-fbdev-sm712fb-fix-crashes-and-garbled-display-during.patch
total: 0 errors, 0 warnings, 105 lines checked
So I mistakenly assumed the patch doesn't have a problem... It seems
checkpatch.pl
cannot detect fallthroughs in nested switch/case statements? I'm not sure.
Should I
report it to the maintainers of checkpatch.pl?
Anyway, please apply the following patch ASAP.
Thanks,
Tom Li
>From 040fa4e6cc8b338cd845c11fd3efd7394ca55108 Mon Sep 17 00:00:00 2001
From: Yifeng Li
Date: Tue, 2 Apr 2019 20:25:20 +0800
Subject: [PATCH] fbdev: sm712fb: fix memory frequency by avoiding a
switch/case fallthrough.
A fallthrough in switch/case was introduced in f627caf55b8e ("fbdev:
sm712fb: fix crashes and garbled display during DPMS modesetting"),
due to my copy-paste error, which would cause the memory clock frequency
for SM720 to be programmed to SM712.
Since it only reprograms the clock to a different frequency, it's only
a benign issue without visible side-effect, so it also evaded Sudip
Mukherjee's code review and regression tests. scripts/checkpatch.pl
also failed to discover the issue, possibly due to nested switch
statements.
This issue was found by Stephen Rothwell by building linux-next with
-Wimplicit-fallthrough.
Reported-by: Stephen Rothwell
Fixes: f627caf55b8e ("fbdev: sm712fb: fix crashes and garbled display during
DPMS modesetting")
Signed-off-by: Yifeng Li
---
drivers/video/fbdev/sm712fb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/video/fbdev/sm712fb.c b/drivers/video/fbdev/sm712fb.c
index 1e2503b52c6f..f1dcc6766d1e 100644
--- a/drivers/video/fbdev/sm712fb.c
+++ b/drivers/video/fbdev/sm712fb.c
@@ -898,6 +898,7 @@ static int smtc_blank(int blank_mode, struct fb_info *info)
case 0x712:
smtc_seqw(0x6a, 0x16);
smtc_seqw(0x6b, 0x02);
+ break;
case 0x720:
smtc_seqw(0x6a, 0x0d);
smtc_seqw(0x6b, 0x02);
--
2.20.1
Re: [PATCH v2 7/7] MAINTAINERS: sm712fb: list myself as one maintainer.
On Sun, Mar 31, 2019 at 08:08:01PM +0100, Sudip Mukherjee wrote: > Technically I donot have any problem with this, you seem to know more > about SM712 than I know. But Teddy Wang is also an existing maintainer > and I think there should be an ack from him before this is accepted. Okay, I'll write a personal mail to Teddy. I think it could be a separate patch to allow more time for communication, but the problem is that, if the MAINTAINERS and the new changes are not merged at the same time, users who have problems may unable to see my name and E-mail address for reporting problems. I think I can send PATCH v3 early for your review, meanwhile I can write a personal E-mail to Teddy. > On Fri, Mar 22, 2019 at 01:17:59PM +0800, Yifeng Li wrote: > > SILICON MOTION SM712 FRAME BUFFER DRIVER > > +M: Tom Li > > Sorry, I am confused. Is your name "Tom Li"? Then why is your > Signed-off-by: and From: name different? Sorry for the confusion. "Tom Li" is my common name. I use this name both in offline and online activities, and I've used it to contribute various patches to free and open source projects and mailing list discussions as well. But the "Signed-off-by" name is the legal name. Linux Kernel's "Developer's Certificate of Origin" requires the use of legal name in sign-offs. I think I should write my name as `Yifeng "Tom" Li` in MAINTAINERS to avoid the confusion. Thanks for your code review, I've noted your suggestions and I'll send PATCH v3 soon. Thanks, Tom Li
Re: [PATCH v2 5/7] Documentation: fb: sm712fb: add information mainly about 2D.
On Sun, Mar 31, 2019 at 07:54:28PM +0100, Sudip Mukherjee wrote: > On Fri, Mar 22, 2019 at 01:17:57PM +0800, Yifeng Li wrote: > > +video controllers. This series of video controller is a legacy from ~1998, > > +and was used on many classic, "prehistoric" laptops from 1998-2004, such as > > +IBM Thinkpad S30 and 240X. It was also used on some servers, industrial > > +computers, x86 and non-x86 embedded devices where only basic graphics was > > +needed. > > I think this is wrong. Loongson 3A Notebook was released around 2011-2012 > and had SM712. "This series of video controller is a legacy from ~1998 and was used on many classic, prehistoric laptops from 1998-2004" is an objective fact. Even if they have been used on newer hardware, it doesn't automatically make the original statement false. But I agree that the description gives incomplete information, I think this paragraph should be reworded for clarity. I would change the description to, > "sm712fb" is a graphics framebuffer driver for Silicon Motion SM710 (LynxEM), SM712 (LynxEM+), and SM720 (Lynx3DM, Lynx3DM+, aka. LynxEM4+) series of video controllers. > This series of video controllers is a legacy product from ~1998, they are designed to be primarily used on low-power mobile systems running Windows 95/ 98/NT/2000, some examples are HP OmniBook XE2 (2000), Panasonic TOUGHBook 28 (2002), FLORA 210W NL3 (2003), Sony Vaio VGN-U50 (2004) OQO Model 01 (2004). > After 2004, they continued to be used on some non-x86 systems, including PowerPC and MIPS. It also saw applications on embedded devices, servers, industrial computers, embedded devices, where low-power operation and/or only basic graphics was needed. > Notably, Lemote YeeLoong 8089, a MIPS laptop based on the Chinese Loongson [...] I think it would be enough. BTW, most Loongson 3A notebooks don't use SM712. I don't know that there are Loongson 3A notebooks that are still using SM712 graphics chip, do you have one? Could you tell me its model number? > > +The first feature is planned to be implemented soon, but the maintainer > > +does not receive any monetary or hardware support from any company or OEMs, > > +and he has to purchase a test platform personally. The 1998's hardware > > +still costs 200 USD+, so don't expected an ETA. If you have a Big-Endian > > +platform and willing to help testing, please contact the maintainer, > > thanks! > > I am not sure why will you want to mention about monetary or hardware > support. Maintainers are supposed to work voluntarily. I agree, I will reword it. > > +Other VGA modes, dual-head, or hardware cursor support should be possible > > to > > +implement, but parts of the code must be rewritten, and there's little > > demand > > +for them on this legacy (retro?) platform, so there's no plan to implement > > them. > > +If you have a genuine need for them, please contact the maintainers. > > If there is any need for new features then I think the plan should be to > make a drm driver. That's the plan. I will reword. > There is a MAINTAINERS file to list the maintainers. There is no need to > add that in documentation. I see. Thanks, Tom Li
[PATCH 0/1] KB3310B MFD Driver for Lemote Yeeloong laptops.
This is a resend of the first patch ("[PATCH v3 1/7]") in the patchset
"[PATCH v3 0/7] Preliminary Platform Driver Support for Lemote Yeeloong
Laptops" [0], which introduced a new MFD driver.
The original patchset never arrived to Lee's inbox. so I resend the
patch to facilitate review.
It features the following...
* Create a MFD driver for KB3310B controller, and move the original
KB3310B controller code from mips/loongson64 to our new MFD driver.
This needs to be reviewed by the MFD subsystem maintainer before the
following can proceed.
* Subdrivers - hwmon, battery, backlight, lcd and hotkey are registered
as MFD cells in the MFD driver. It means onlf the MFD driver is resposible
to register the upcoming subdrivers, the core board files mips/loongson64/
will not contain unrelated code.
[0]
https://lore.kernel.org/linux-mips/20190306120113.648-1-to...@tomli.me/T/#m2a6e18afbb62ceb535a859181d8485916b30a63f
Thanks,
Yifeng Li (1):
mfd: yeeloong_kb3310b: support KB3310B EC for Lemote Yeeloong laptops.
MAINTAINERS | 7 +
drivers/mfd/Kconfig | 10 ++
drivers/mfd/Makefile | 1 +
drivers/mfd/yeeloong_kb3310b.c | 200 +
include/linux/mfd/yeeloong_kb3310b.h | 211 +++
5 files changed, 429 insertions(+)
create mode 100644 drivers/mfd/yeeloong_kb3310b.c
create mode 100644 include/linux/mfd/yeeloong_kb3310b.h
--
2.20.1
[PATCH 1/1] mfd: yeeloong_kb3310b: support KB3310B EC for Lemote Yeeloong laptops.
From: Yifeng Li Lemote Yeeloong is a laptop powered by Loongson 2F MIPS processor, primarily a demo platform for hobbyists and developers. It uses an ENE KB3310B Embedded Controller with customized firmware to implement hardware and power management. A monolithic platform driver code for those functionality has existed out-of-tree for many years. This commit creates a MFD driver for the EC chip on Yeeloong laptop to isolate EC-related code from core MIPS code, and serves as the foundation of various subdrivers. NOTE: A regmap could be the reasonable abstraction for I/O, but (1) it requires some additional refactoring to convert the shutdown/wakeup routines in board files to subdrivers, and (2) we're only using simple reads/writes, repmap only adds boilerplates to the existing files without additional benefits. We simply export the EC-related functions for now, until we come back to refactor the board files. Signed-off-by: Yifeng Li --- MAINTAINERS | 7 + drivers/mfd/Kconfig | 10 ++ drivers/mfd/Makefile | 1 + drivers/mfd/yeeloong_kb3310b.c | 200 + include/linux/mfd/yeeloong_kb3310b.h | 211 +++ 5 files changed, 429 insertions(+) create mode 100644 drivers/mfd/yeeloong_kb3310b.c create mode 100644 include/linux/mfd/yeeloong_kb3310b.h diff --git a/MAINTAINERS b/MAINTAINERS index 51029a425dbe..208f19801a23 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -16839,6 +16839,13 @@ S: Maintained F: Documentation/input/devices/yealink.rst F: drivers/input/misc/yealink.* +YEELOONG ENE KB3310B MFD DRIVER +M: Tom Li +L: linux-m...@vger.kernel.org +S: Maintained +F: drivers/mfd/yeeloong_kb3310b.c +F: include/linux/mfd/yeeloong_kb3310b.h + Z8530 DRIVER FOR AX.25 M: Joerg Reuter W: http://yaina.de/jreuter/ diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index f461460a2aeb..a6da8cce72fc 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -1872,6 +1872,16 @@ config MFD_STM32_TIMERS for PWM and IIO Timer. This driver allow to share the registers between the others drivers. +config MFD_YEELOONG_KB3310B + bool "ENE KB3310B Embedded Controller on Lemote Yeeloong laptops" + depends on (MIPS && LEMOTE_MACH2F) || COMPILE_TEST + select MFD_CORE + help + Select this option to enable ENE KB3310B Embedded Controller + driver used on Lemote Yeeloong laptops, providing power, battery + and backlight services. This is a mandatory dependency for + Lemote 2F systems. + menu "Multimedia Capabilities Port drivers" depends on ARCH_SA1100 diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 12980a4ad460..a3446ce7c384 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -224,6 +224,7 @@ obj-$(CONFIG_MFD_HI655X_PMIC) += hi655x-pmic.o obj-$(CONFIG_MFD_DLN2) += dln2.o obj-$(CONFIG_MFD_RT5033) += rt5033.o obj-$(CONFIG_MFD_SKY81452) += sky81452.o +obj-$(CONFIG_MFD_YEELOONG_KB3310B) += yeeloong_kb3310b.o intel-soc-pmic-objs:= intel_soc_pmic_core.o intel_soc_pmic_crc.o obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o diff --git a/drivers/mfd/yeeloong_kb3310b.c b/drivers/mfd/yeeloong_kb3310b.c new file mode 100644 index ..423d5ced2f65 --- /dev/null +++ b/drivers/mfd/yeeloong_kb3310b.c @@ -0,0 +1,200 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +/* + * MFD driver for ENE KB3310B embedded controller on Lemote Yeeloong laptops + * + * Copyright (C) 2008 Lemote Inc. + * Author: liujl , 2008-04-20 + * + * Copyright (C) 2019 Yifeng Li + * Author: Yifeng Li + * + * This is a MFD driver for the ENE KB3310B Embedded Controller for Lemote + * Yeeloong laptops to provide utility functions to access the chip from + * subdrivers, and handle events and interrupts in board files. This is a + * special-purpose driver, and it's only used on Lemote Yeeloong laptops, + * and is a mandatory dependency. + * + * NOTE: A regmap could be the reasonable abstraction for I/O, but (1) + * it requires some additional refactoring to convert the shutdown/wakeup + * routines in board files to subdrivers, and (2) we're only using simple + * reads/writes, repmap only adds boilerplates to the existing files without + * additional benefits. We simply export the EC-related functions for now, + * until we come back to refactor the board files. + */ + +#include +#include +#include +#include +#include + +#include + +#ifdef pr_fmt +#undef pr_fmt +#endif + +#define DRV_NAME "yeeloong_kb3310b" +#define pr_fmt(fmt) DRV_NAME ": " fmt + +/* + * Most drivers, such as battery or backlight drivers, uses the I/O ports to + * access the Index Registers to obtain hardware status and information from + * EC chip. + */ +static struct kb3310b_chip *kb3310b_fwinfo; + +static const str
Re: Is it possible to reset graphics controller on reboot in a framebuffer driver?
On Fri, Mar 08, 2019 at 10:13:58AM +0100, Geert Uytterhoeven wrote: > Hi Tom, > > If the kernel just crashes, of course all of that doesn't happen. > Is your graphics card reset when the reset button is pressed, or only on > cold power on? It's a laptop, so it doesn't have a reset button. I've tried reboot=cold/warn but apparently it doesn't make any difference. So I think only a cold boot can reset the graphics card. > On Thu, Mar 7, 2019 at 10:38 PM Tom Li wrote: > > Nevertheless, does it mean there's no way to prevent it from happening if > > the > > user issues a emergency reboot? Like an automatic reboot after a kernel > > panic, > > or a SysRq-B reboot. > > If Linux performs a reboot, it calls the shutdown handlers. > I think that includes reboot on panic, or SysRq-B, but I'd have to check to > be 100% sure. Okay, glad to hear that. If it works for SysRq-B or panic reboot, I think it would be enough. After all, hard kernel crashes are rare nowadays, and most crashes are hard lockups. In case it happens, the user just presses the power button to halt. NMI watchdog reset is still an issue though, but I don't think people who are playing with those ~1999-2002 hardware (like this one) is actually using it. Anyway, In this case, I think putting a special note in the documentation, and raising a warning in dmesg (WARNING: LCD output may be corrupted on reset, read ./Documentation/fb/sm712fb.txt!) would be adequate. Thanks, Tom Li
Re: Is it possible to reset graphics controller on reboot in a framebuffer driver?
On Thu, Mar 07, 2019 at 10:39:23AM +0100, Geert Uytterhoeven wrote:
> On Thu, Mar 7, 2019 at 10:00 AM Jani Nikula
> wrote:
> > It's possible to do this using a reboot notifier. I am not sure if there
> > are better ways to achieve the same, but there's at least one example of
> > using reboot notifiers to achieve the exact same goal.
> >
> > See drivers/video/fbdev/aty/atyfb_base.c, look for
> > register_reboot_notifier().
>
> Or a shutdown handler, which is more device-centric?
> (cfr. "[3/4] fbdev: atafb: Fix broken frame buffer after kexec",
> https://patchwork.kernel.org/patch/10814381/).
>
> Gr{oetje,eeting}s,
Thanks, I knew reboot_notifier but I thought it feels "hacky" to use it in a
device driver, shutdown() handler looks better.
Nevertheless, does it mean there's no way to prevent it from happening if the
user issues a emergency reboot? Like an automatic reboot after a kernel panic,
or a SysRq-B reboot.
Tom Li
Is it possible to reset graphics controller on reboot in a framebuffer driver?
Hi all. As you may have noticed, recently I've been working on a reworked version of sm712fb, and planned to convert it to a DRM/KMS driver. Besides using it on embedded/non-x86 systems, I thought it would be a good idea to support histrocial x86 laptops with this VGA chipset as well, so I've acquired a machine for testing. However, soon I found a nasty problem. The BIOS does not reset the chip on boot! Like most graphics controller of that era, sm712 chipset has a VGA compatible mode and a 2D framebuffer mode. The power-on default is VGA. The BIOS writer just assumed this, and does nothing to reinitialize it. If one uses the framebuffer driver under Linux, once the machine reboots, the entire LCD panel becomes a piece of garbage. AFAIK, the framebuffer driver would be running throughout the kernel's life- cycle, is it really possible to workaround this issue by restoring on VGA state upon reboot? Thanks, Tom Li
Re: [PATCH v2 1/7] mfd: yeeloong_kb3310b: support KB3310B EC for Lemote Yeeloong laptops.
On Tue, Mar 05, 2019 at 11:50:51PM +, Paul Burton wrote:
> Hi Tom,
>
> Overall I think this is much better than the older out of tree platform
> driver - thanks for cleaning it up.
I'm glad to hear that.
> One general comment - it would be good to run the patches through
> ./scripts/checkpatch.pl & fix up any warnings it gives unless you have a
> good reason to disagree with them.
The entire patchset has already been checked by checkpatch.pl, and all
generated warnings have been corrected before submission. So I don't think
the script can help me improving the style beyond this point. Thanks for
pointing out all the additional style issues.
> Nit: the lines of asterisks aren't part of the kernel's general comment
> style & I think it would looks cleaner to remove them.
Sure, I will remove these asterisks banners.
In fact I learned to use these asterisks from other kernel drivers to
separate logical sections... and as a said, checkpatch.pl does not generate
a warning about it, so I suspect that, historically, a clear policy about
asterisks banners was never enforced. After finish working on these patches,
perhaps it would a good time to patch checkpatch.pl and add a check for it?
> I'm not sure I follow why the power management code prevents use of
> regmap?
>
> Are you talking about the wakeup_loongson() function? Perhaps it would
> make sense for the suspend code to be part of one of the possible
> subdrivers you mention. The lemote-2f seems to be the only system that
> provides an implementation of wakeup_loongson() so perhaps a driver
> could instead just register its own struct platform_suspend_ops & avoid
> the need for code in arch/mips to care about the EC.
I'll reword my misleading commit message about using regmap.
Let me clarify it - nothing prevents the use of regmap. I originally thought
it would be a good idea since the manual locking can be eliminated.
But later I realized it would mean more changes. In particular, not only pm.c
needs to be a new subdriver, the shutdown code in ml2f_reboot() also calls
ec_write(), and it needs to be converted to a subdriver in drivers/power/reset/
too. Yes, it would be good to refactor these code and move them to a more
suitable place, but currently, the primary goal is to add platform drivers,
not to touch functions I've just fixed and possibly introduce new regressions...
As you've seen, there is a TODO item in [PATCH 6/7] in this series, it says the
CS5536 code also needs some major refactoring: the GPIO needs to be changed,
the clocksource driver needs to be merged with the clockevent driver under
drivers/clocksource, etc. I think we can, as well, save the pm.c/reset.c for
later. Currently, I think it's better to focus on the platform drivers first.
If you still have objections on it, please let me know.
> > +#define DRV_NAME "yeeloong_kb3310b: "
>
> Defining pr_fmt() would be cleaner - you wouldn't need to manually
> include DRV_NAME in your messages later.
Noted.
> > +static struct kb3310b_chip *kb3310b_fwinfo;
> > +
> > +static const struct mfd_cell kb3310b_cells[] = {
> > + {
> > + .name = "yeeloong_sci"
> > + },
> > + {
> > + .name = "yeeloong_hwmon"
> > + },
> > + {
> > + .name = "yeeloong_battery"
> > + },
> > + {
> > + .name = "yeeloong_backlight"
> > + },
> > + {
> > + .name = "yeeloong_lcd"
> > + },
> > + {
> > + .name = "yeeloong_hotkey"
> > + },
>
> Nit: I think it'd look cleaner if you remove the newlines within each
> array entry, eg:
>
> { .name = "yeeloong_sci" },
> { .name = "yeelong_hwmon" },
> ...
Yes.
> > +
> > +static DEFINE_SPINLOCK(kb3310b_command_lock);
>
> Since this is only used in kb3310b_query_seq() could you just declare it
> (still static) inside that function?
No problem.
Thanks for your review, I'll correct all the issues above and send v3 today,
if there's no addition problem in v3, I will start sending the actual platform
drivers (battery/hwmon/etc) for the next round of review.
P.S: This time, I hope Lee Jones, as the MFD subsystem maintainer, has
received my mail and my patch, including this one. Unfortunately, all signs
indicated he hasn't received it. Jones, if you have received this mail but
currently too busy to review, please reply to confirm, thanks!
Sincerely yours,
Tom Li
MFD/Platform drivers for Yeeloong laptop submitted, please review
Hello, Paul & Jones. I've submitted the first series of patches [0] for the platform drivers on Lemote Yeeloong MIPS laptop to the mailing list, this series is required before the following mainlining can be proceed. The first patch of this series is a MFD driver, which is needed to be reviewed by Jones. The rest of the patches is the required changes to the board files in arch/mips/loongson64/, which is needed to be reviewed for inclusion into Linux/MIPS. In addition, there is also a trivial bugfix for suspend/resume, which is needed to be queued into mips-fixes. I understand I shouldn't have sent this mail because it only adds noise to the mailing list, but in this case, unfortunately, it seems Jones never received the original patch, perhaps my mail was classified as spam or rejected by his mail server, and in fact he didn't even realized the existence of it... [2] Jones: I sincerely hope you've receive this mail. Also, is there a mailing list which can be used to submit MFD drivers rather than CCing you personally? (in case the mail has been dropped again) Thanks for your time. Thanks, Tom Li [0] https://lore.kernel.org/linux-mips/20190304222848.25037-1-to...@tomli.me/T/#t [1] https://lore.kernel.org/linux-mips/20190304220022.20682-1-to...@tomli.me/T/#u [2] https://lore.kernel.org/linux-mips/20190302175334.5103-1-to...@tomli.me/T/#maabd498c419c13a5435231303ef803a71451af8b
Re: [PATCH 1/7] mfd: yeeloong_kb3310b: support KB3310B EC for Lemote Yeeloong laptops.
On Mon, Mar 04, 2019 at 10:14:37AM +, Lee Jones wrote: > This patch isn't in my Inbox. > > Who was it sent to? I sent the patch to you and linux-mips. This patch series is a MIPS platform driver, and the first patch in this series creates a MFD driver which is needed to be reviewed by you. Unfortunately, somehow you didn't receive it, perhaps it was rejected by the mail server? Hasn't the bot tested the patch, weeks of time will be wasted... Is there a mailing list I can use to send MFD patches for you to review? Thanks, Tom Li
Re: [PATCH 1/7] mfd: yeeloong_kb3310b: support KB3310B EC for Lemote Yeeloong laptops.
>drivers/mfd/yeeloong_kb3310b.c: In function 'kb3310b_read': > >> drivers/mfd/yeeloong_kb3310b.c:70:2: error: implicit declaration of > >> function 'outb' [-Werror=implicit-function-declaration] > outb((reg & 0xff00) >> 8, KB3310B_IO_PORT_HIGH); > ^~~~ > >> drivers/mfd/yeeloong_kb3310b.c:72:8: error: implicit declaration of > >> function 'inb' [-Werror=implicit-function-declaration] > val = inb(KB3310B_IO_PORT_DATA); >^~~ Nice bot. I'll send out PATCH v2 soon with this fixed. Cheers, Tom Li
Re: [RFC 1/1] tty: vt.c: Fix TIOCL_BLANKSCREEN VT console blanking if blankinterval == 0
> Wouldn't this problem be fixed simply by the following: > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c > index 9646ff63e7..41b17c4b5a 100644 > --- a/drivers/tty/vt/vt.c > +++ b/drivers/tty/vt/vt.c > @@ -4151,8 +4151,6 @@ void do_blank_screen(int entering_gfx) > return; > } > > - if (blank_state != blank_normal_wait) > - return; > blank_state = blank_off; > > /* don't blank graphics */ > > I just can't find a logical reason for that conditional return. > > > Nicolas I see. I was concerned about the possibly unwanted effect if the function is reentered, but it seems that there's no logic reason to worry at all. I'll send a new patch according to your suggestion. Thanks, Tom Li
Re: [PATCH 0/1] mips: loongson64: move EC header to include/asm/mach-loongson64
> Hi, > > Yifeng Li (1): > > mips: loongson64: move EC header to include/asm/mach-loongson64 > > This probably should be MFD driver under drivers/mfd. It's a longer > road, though... > > A. The problem of converting it to a MFD driver, is that there's still something doesn't fit together. The core CPU idle code in loongson64/common calls the platform code in arch/mips/loongson64/lemote-2f to set up an interrupt handler in i8259, and inspect if the source of interrupt is an lid open event by querying the ENE KB3310B controller. But normally an MFD driver is only used to share the underlying hardware between multiple drivers in each subsystem, not to provide shared utilily functions. But after reading drivers/mfd for a while, apparently we do have drivers that "EXPORT_SYMBOL()" to other parts of the kernel. So I think it's a matter of fact that can be discussed with MFD maintainers. So yes, I'm converting all the EC code to a MFD driver. I have withdrawn this patch, do not merge it. The new MFD patch is coming soon. Thanks, Tom Li signature.asc Description: PGP signature
Re: [RFC] On the Current Troubles of Mainlining Loongson Platform Drivers
On Mon, Feb 11, 2019 at 10:13:00AM -0200, Alexandre Oliva wrote: > On Feb 8, 2019, Tom Li wrote: > > > found Alexandre Oliva has stopped maintaining his tree > > ?!? > > I still merge and tag every one of Torvalds' and Greg KH's releases into > the loongson-community tree, resolving trivial conflicts and trying to > verify that it at least builds and passes a smoke test on actual > hardware (after Linux-libre deblobbing, but still). What is the current link to your repository? The original one from Lemote is now a 404, and the GitHub one has been "archived". > Two issues that bother me a bit are frequent failure to reboot/poweroff, > that's been around since around 4.10, and, more recently, a very slow > system overall, that's been present since 4.20. I haven't checked > whether they're caused by changes in the loongson-community tree that I > still carry, or if they're already present in upstream releases. > > I've already tried to bisect the former, but since the issue is > intermittent, that proved to be too tricky and time-consuming for me. > > I haven't tried to bisect the latter yet. You cannot bisect it. Because I've tried it for 24 hours without sleep, but failed. ;-) We've just identified and confirmed the source of the shutdown problem a few days ago on this mailing list. It's related to a PREEMPT race condition in the cpufreq framework that triggers a timing-dependent, probabilistic lockup in the i8259 PIC driver. And it was first noticed by Ville Syrjälä on a x86 system, my debugging rediscovered his findings. You can read the full investigation at here: https://lkml.org/lkml/2018/11/13/857 You can pick up the patch from: https://lore.kernel.org/lkml/20190207205812.ga11...@darkstar.musicnaut.iki.fi/ A patch has been authored and submitted by Aaro Koskinen, but currently it seems stuck in the mailing list because the maintainer worries about regression and asks for testing on more MIPS systems, although we believe it's a trivial patch. In addition, I've discovered and fixed another bug preventing the machine from shutting down, this patch has already merged into the Linus tree. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8a96669d77897ff3613157bf43f875739205d66d > and, more recently, a very slow system overall, that's been present since > 4.20. The mainline framebuffer driver doesn't have any hardware drawing, printing even a single line on the console is required a full screen redraw via memory- mapped I/O over the slow PCI interface. I have rebased the 2D acceleration code against the latest upstream driver and sent the patch to the fbdev mailing list on February 2nd. But the maintainers seem to be working on other tasks and the patches (including mine and others') on the list didn't receive any response. It's avaliable from https://patchwork.kernel.org/project/linux-fbdev/list/?series=75029 > I've considered leaving the loongson-community tree behind and using > upstream releases, but every time I was about to do that, something else > came up that led me to keep at it. I think the last such occurrence was > the removal of the video driver (later reintroduced as a new driver). > So I've kept at it ;-) I'll continue working on upstreaming these out-of-tree drivers as my personal project. I hope you'll be able to use a fully-functional machine with the mainline kernel soon, my current target is Linux 5.3. Cheers, Tom Li signature.asc Description: PGP signature
Re: [RFC] On the Current Troubles of Mainlining Loongson Platform Drivers
> To address the particular quote you give from Dmitry Torokhov on the > yeeloong_hotkey driver - just because the driver as-is includes a bunch > of non-input related things doesn't mean that it should or has to. From > a look at the 2009 submission it seems to mix a bunch of policy into the > kernel which really ought to be elsewhere. Generally the input driver > reports that a key was pressed & something in userland decides what to > do with it, whereas this driver seems to attempt to bypass that & prod > at unrelated hardware all by itself. Sure, the hotkey driver has some problems in its current shape. I think the existing code makes some hotkeys on the keyboard behave like a hardware switch to order to implement rfkill hardblock, and also controls the video output switch. I think I need to investigate it further. > Personally I see no reason for that hwmon driver to live under > drivers/platform/mips rather than drivers/hwmon. All that does is bypass > the attention of the drivers/hwmon/ maintainers who would be best placed > to offer input & ensure the driver is actually any good. Yes. I think if our conclusion is "drivers/platform/mips is not a good idea", it should be moved to the appropriate category in the future. > I think that question should prompt another - if we have maintainers for > various driver subsystems, why not place the drivers under their care in > the already established directories? > > Thanks, > Paul Thanks for your reply, I'm fully agree with your comments about platform drivers. I find reorganization of the current Yeeloong platform driver is relatively easy, since it only involves one machine, I'm already working on it. If future developers find it's difficult to support new machines, we can simply have more discussion, reorganize the existing hierarchy further, and make incremental changes. Cheers, Tom Li Beijing GNU/Linux User Group. signature.asc Description: PGP signature
[RFC] On the Current Troubles of Mainlining Loongson Platform Drivers
it also affects on drivers in unrelated-subsystems, thus creating many troubles on coordination. Even worse, they are only designed to interact on each other. For example, closing the lid may deactivate the screen and changes the behavior of a light sensor, a power button, and a 3G modem and their respective LED indicators. From a EC driver perspective, it's easy to manage, but harder to do so independent drivers. * Some computers have different hardware functionatilies, e.g. different screen sizes, types of batteries, or keymap, but often share a single family of the EC chipset and they can be controlled in a similar way, or have similar characteristics. Using single platform drivers is easier to add support of a new machise, or to reuse code among different computers - simply extend the existing platform driver is enough. In our case, It's expected to support more Loongson PCs of the same family, such as Yeeloong laptops, MengLoong machines and FuLoong Mini PCs. The platform drivers in drivers/platform/x86 is representative for the points above. For example, Greg Kroah-Hartman's samsung-laptop.c. Argument AGAINST platform driver * Loongson is only a subfamily of MIPS systems. Unlike x86, where there is a massive number of platforms which need custom platform drivers, the vast majority of MIPS are servers/workstations or embedded systems, which does not use platform drivers. Introducing platform drivers will encourage unrelated drivers to be submitted to the Linux/MIPS mailing list, creating more pressure to a small team like Linux/MIPS. * Linux/MIPS is specialized in maintaining core MIPS code. The developers should not be forced into reviewing and maintaining unrelated subsystems, such as battery, LEDs, or hotkeys, which they are not responsible to. They should be reviewed by the maintainers of their respective subsystem, this also encourages higher-quality drivers. * x86 platform drivers are maintained independently from core x86 code. in a separate tree, and has its own community, platform-driver-...@vger.kernel.org. However, there is nothing is this in Linux/MIPS. * The code reuse and coordination problem is simply an excuse, it can be solved by providing a generic EC interface in arch/mips/loongson64 for each driver in its respective subsystem. Monolithic Kitchen-sink platform drivers are anti-patterns and they shouldn't be developed. In conclusion, in my opinion, although having a central location for Loongson platform driver is preferable, but consider the current circumstances, it's a bad idea, and in particular, bad for Linux/MIPS maintainers. I think the best way of doing it under the current system is splitting the drivers to their respective subsystems, and put the EC-related code in arch/mips/loongson64 and export the symbols. If there's consensus on it, I think I can starting working on it immediately. Nevertheless, I think a longterm solution is needed to facilitate the development of lots more platform drivers of Loongson mainches. I've noticed that Google has a similar problem developing their own platform drivers for Chromebooks, their solution is creating drivers/platform/chrome, so they can work on i2c, lightbar, LED backlight, etc, drivers tightly-coupled with the Embedded Controller in a central location, with its dedicated tree chrome-platform.git, without disturbing the ARM developers by sending unrelated drivers. I think we should adopt this model, having a drivers/platform/loongson should make the development easier. Unsolved problems still remains, however. First, how to solve the coordination problem? Linux/MIPS is a relatively low-volume and slow list, what to do if the unmerged MIPS patches becomes a blocker for other drivers? Second, we already have a hwmon driver under drivers/platform/mips, should we put a warning that says new drivers under this directory shouldn't be written? Third, if we agree to create drivers/platform/loongson in the future, how can we ask the maintainers of other subsystems to review the drivers in it? I think Google bypassed the problem because they have lots of kernel developers, but not us. Does anyone know more about Chromebook's development procedures? Also, if we want drivers/platform/loongson and its own tree, what are the procedures to get them created? All comments, suggestions, criticisms, etc, are appreciated. Thanks, Tom Li Beijing GNU/Linux User Group [1] 2009/12: https://lore.kernel.org/linux-mips/1259679137.12571.4.camel@falcon/ [2] 2010/05: https://lore.kernel.org/linux-mips/cover.1274622624.git.wuzhang...@gmail.com/ [3] 2017/11: https://lore.kernel.org/linux-mips/20171112063617.26546-3-jiaxun.y...@flygoat.com/ [4] https://www.spinics.net/lists/platform-driver-x86/ signature.asc Description: PGP signature
Re: [REGRESSION 4.20-rc1] 45975c7d21a1 ("rcu: Define RCU-sched API in terms of RCU for Tree RCU PREEMPT builds")
On Tue, Feb 05, 2019 at 11:58:09AM +0200, Aaro Koskinen wrote: > Can you try below fix? It works on my Loongson. Hello Aaro, thanks for your response. But in case you've missed the original thread, please check it at: https://lkml.org/lkml/2018/11/13/857 My problem is NOT about how to fix the problem on Loongson (or x86): the patch in the original thread (only has one-line-of-code, simply changes timing of cpufreq_core_init), or your patch, is indeed working. But they are workarounds, the real issue is the race condition in cpufreq. My question is 1. What's the progress and current consensus about how the underlying problem can be fixed. 2. If there's no consensus since November, is it possible that we land a hotfix patch to linux-stable as a temporary workaround? Cheers, Tom Li.
Re: [REGRESSION 4.20-rc1] 45975c7d21a1 ("rcu: Define RCU-sched API in terms of RCU for Tree RCU PREEMPT builds")
On Tue, Nov 13, 2018 at 03:54:53PM +0200, Ville Syrjälä wrote:
> Hi Paul,
> After 4.20-rc1 some of my 32bit UP machines no longer reboot/shutdown.
> I bisected this down to commit 45975c7d21a1 ("rcu: Define RCU-sched
> API in terms of RCU for Tree RCU PREEMPT builds").
>
> I traced the hang into
> -> cpufreq_suspend()
> -> cpufreq_stop_governor()
> -> cpufreq_dbs_governor_stop()
>-> gov_clear_update_util()
> -> synchronize_sched()
> -> synchronize_rcu()
>
> Only PREEMPT=y is affected for obvious reasons, but that couldn't
> explain why the same UP kernel booted on an SMP machine worked fine.
> Eventually I realized that the difference between working and
> non-working machine was IOAPIC vs. PIC. With initcall_debug I saw
> that we mask everything in the PIC before cpufreq is shut down,
Hello Paul & Ville.
I'm not a kernel hacker, just a n00b playing with various non-x86
systems, but I've been forced getting into kernel hacking due to
the same issue.
Since February, I'm working on porting some trivial out-of-tree
drivers to the upstream, and noticed my Yeeloong 8089D, a machine running
the Loongson 2F 64-bit MIPS-III processor, will completely hang during
reboot or shutdown. I tried bisecting it for 24 hours without sleep, but
the attempt had failed.
I managed to narrow it in-between 4.19 and 4.20, most unusual thing I've
observed was its probabilistic nature. The chance of triggering it seems
getting progressively higher since 4.20, making pinpointing the specific
commit difficult, finally with a 100% chance since Linux 4.19.
I initially suspected a bug in the platform driver, later I also tried
to disable various kernel hardening options, all without success. At this
point I've realized, because it has shown to be probabilistic, it must be
a race condition, and since it's an uniprocessor system, it must be the
CPU scheduler getting preempted somehow. Disabling CONFIG_PREEMPT makes
the issue go away. I tried to add various preempt_disable() in the platform
driver but didn't work.
Finally I've hooked up a netconsole and started adding printk()s.
[ 23.652000] loongson2_cpufreq loongson2_cpufreq: shutdown
[ 23.656000] loongson-gpio loongson-gpio: shutdown
[ 23.66] migrate_to_reboot_cpu()
[ 23.664000] syscore_shutdown()
[ 23.668000] PM: Calling i8259A_shutdown+0x0/0xa8
[ 23.672000] PM: Calling irq_gc_shutdown+0x0/0x88
[ 23.672000] PM: Calling cpufreq_suspend+0x0/0x1a0
[ 23.672000] cpufreq_suspend()
[ 23.672000] cpufreq_suspend: Suspending Governors
[ 23.672000] cpufreq_stop_governor()
[ 23.672000] cpufreq_stop_governor: for CPU 0
Looks like something in the core cpufreq code? So I tried searching
"cpufreq_stop_governor()" at LKML... Oops!
So it must be the same issue. To summary, the issue exists on all Linux
kernels since 4.20-rc1, and the chance of triggering it, is now 100%.
What is the current progress of purposed solutions? If a complete solution
is still work-in-progress, could we simply submit a hotfix into the linux-stable
trees, so at least the issue can be temporarily solved? What can I do to help
testing?
Thanks!
Tom Li
Re: [PATCH 0/8] fbdev: sm712fb: implement 2D acceleration w/ cleanups.
> Since you care about this driver, considered converting it to a drm > display driver? You can still have all the acceleration and stuff, the > fbdev compat mode in drm is rather flexible. > -Daniel Yes, I know fbdev is now in maintenance-only mode, reimplementing it on top of DRM is on my roadmap, including rewriting a saner version of the modesetting code. The current version is beyond repair. I'll start working on it once I managed to purchase the hardware for testing. But currently my objectively is to have something usable during the transitional period. The major user of the chipset was Yeeloong 8089D laptop - a choice for many non-x86 and MIPS hobbyists for experimentation. Getting the current version of the patch merged would solve the immediate usability issues, and then trying to get some platform drivers merged. And after completing the preliminary upstreaming process, since then, can start working on a DRM version of the driver, and possibly eventually remove the fbdev version of sm712fb entirely. The 2D acceleration implementation is strictly no more than 200 lines, the vast majority of the code insertions are comments and documentation changes ranting about the known issues of the driver/hardware, nothing non-trivial is added and I think the changeset is manageable and would not be a burden for the fbdev maintainers, and I can grarantee that I will not add any other new features to this driver. Cheers, Tom Li
Re: [BUG] What is "__ptrval__" in my dmesg logs? Bad "%p" expansion?
This is not a bug, rather, this is a security feature that fixes the original behavior, which is now considered an infoleak vul- nerability. Currently, the address of internal data structures are protected by Kernel Address Space Layout Randomization (KASLR), it forces attackers to bruteforce the location they need to overwrite, thus together with W^X mappings, increases the difficulty of exploiting the kernel. However, showing values of raw pointers will reveal an address of a known internal data structure, allowing an attacker to calculate the location of critical data structure within the kernel, therefore completely defeating the protection by ASLR. This is why disallowing normal users to "dmesg" used to considered a way to improve system security. As a security measure, the value of "%p" is now hidden by default. Happy Hacking, Tom Li
