Re: [PATCH] hid-input: Fix devices that return multiple bytes in battery report
On Fri, Jul 10, 2020 at 8:19 AM Grant Likely wrote:
>
> Some devices, particularly the 3DConnexion Spacemouse wireless 3D
> controllers, return more than just the battery capacity in the battery
> report. The Spacemouse devices return an additional byte with a device
> specific field. However, hidinput_query_battery_capacity() only
> requests a 2 byte transfer.
>
> When a spacemouse is connected via USB (direct wire, no wireless dongle)
> and it returns a 3 byte report instead of the assumed 2 byte battery
> report the larger transfer confuses and frightens the USB subsystem
> which chooses to ignore the transfer. Then after 2 seconds assume the
> device has stopped responding and reset it. This can be reproduced
> easily by using a wired connection with a wireless spacemouse. The
> Spacemouse will enter a loop of resetting every 2 seconds which can be
> observed in dmesg.
>
> This patch solves the problem by increasing the transfer request to 4
> bytes instead of 2. The fix isn't particularly elegant, but it is simple
> and safe to backport to stable kernels. A further patch will follow to
> more elegantly handle battery reports that contain additional data.
>
Applied and tested on 5.8.0-rc4+ (aa0c9086b40c) with a 3Dconnexion
SpaceMouse Wireless (tested connected via USB). Observed the same
behavior Grant reports before the patch. After the patch, the device stays
connected successfully.
Tested-by: Darren Hart
Thanks Grant!
> Signed-off-by: Grant Likely
> Cc: Darren Hart
> Cc: Jiri Kosina
> Cc: Benjamin Tissoires
> Cc: sta...@vger.kernel.org
> ---
> drivers/hid/hid-input.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index dea9cc65bf80..e8641ce677e4 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -350,13 +350,13 @@ static int hidinput_query_battery_capacity(struct
> hid_device *dev)
> u8 *buf;
> int ret;
>
> - buf = kmalloc(2, GFP_KERNEL);
> + buf = kmalloc(4, GFP_KERNEL);
> if (!buf)
> return -ENOMEM;
>
> - ret = hid_hw_raw_request(dev, dev->battery_report_id, buf, 2,
> + ret = hid_hw_raw_request(dev, dev->battery_report_id, buf, 4,
> dev->battery_report_type,
> HID_REQ_GET_REPORT);
> - if (ret != 2) {
> + if (ret < 2) {
> kfree(buf);
> return -ENODATA;
> }
> --
> 2.20.1
>
Re: [Pv-drivers] [PATCH 1/4] x86/vmware: Update platform detection code for VMCALL/VMMCALL hypercalls
> On Aug 18, 2019, at 12:20 PM, Thomas Gleixner wrote: > > While at it could you please ask your legal folks whether that custom > license boilerplate can go away as well? If you’re referring to the GPL boilerplate with “no warranty" and physical address, then yes, as a matter of best practice (at VMware), that can and should all be removed when adding the SPDX identifier - and of course, as you said, be done as a separate patch. Thanks, -- Darren Hart
Re: [PATCH] platform/x86: pmc_atom: Add Lex 3I380D industrial PC to critclk_systems DMI table
On Wed, May 08, 2019 at 03:55:22PM -0700, Darren Hart wrote: > On Wed, May 08, 2019 at 11:20:52AM +0200, Hans de Goede wrote: > > Hi, > > > > On 08-05-19 10:42, Andy Shevchenko wrote: > > > On Wed, May 8, 2019 at 10:48 AM Hans de Goede wrote: > > > > On 07-05-19 22:17, Stephen Boyd wrote: > > > > > Quoting Hans de Goede (2019-05-06 08:05:42) > > > > > > > > I guess this is urgent? > > > > > > > > Somewhat, getting this into e.g. rc2 would be fine too, waiting till 5.3 > > > > would be bad. > > > > > > So, I can do it as a fixes for rc2, just ping me after merge window. > > > > Ok, will do. > > Andy, what is the issue here? If the dependency is in v5.1 we can do a "merge > --ff-only v5.1" in our for-next branch in order to pull it in, that would be > the > same as an immutable branch basically. > A simpler solution for this case would be to issue two PRs to Linus from two different branches. Other subsystems send topic branches, so this isn't out of the ordinary. I have merged the two patches in question from Hans and Steffen to for-next-2. We could send two PRs back to back, with a note to Linus why this is a bit different than usual, and then come back together in our for-next and fixes branches once both are merged and continue as usual. Any concerns with this approach? -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] platform/x86: acer-wmi: Mark expected switch fall-throughs
On Wed, May 08, 2019 at 11:49:34AM -0500, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch
> cases where we are expecting to fall through.
>
> This patch fixes the following warnings:
>
> drivers/platform/x86/acer-wmi.c: In function ‘set_u32’:
> drivers/platform/x86/acer-wmi.c:1378:33: warning: this statement may fall
> through [-Wimplicit-fallthrough=]
> if (cap == ACER_CAP_WIRELESS ||
> ^
> drivers/platform/x86/acer-wmi.c:1386:3: note: here
>case ACER_WMID:
>^~~~
> drivers/platform/x86/acer-wmi.c:1393:12: warning: this statement may fall
> through [-Wimplicit-fallthrough=]
> else if (wmi_has_guid(WMID_GUID2))
> ^
> drivers/platform/x86/acer-wmi.c:1395:3: note: here
>default:
>^~~
> drivers/platform/x86/acer-wmi.c: In function ‘get_u32’:
> drivers/platform/x86/acer-wmi.c:1340:6: warning: this statement may fall
> through [-Wimplicit-fallthrough=]
>if (cap == ACER_CAP_MAILLED) {
> ^
> drivers/platform/x86/acer-wmi.c:1344:2: note: here
> case ACER_WMID:
> ^~~~
> drivers/platform/x86/acer-wmi.c: In function ‘WMID_get_u32’:
> drivers/platform/x86/acer-wmi.c:1013:6: warning: this statement may fall
> through [-Wimplicit-fallthrough=]
>if (quirks->mailled == 1) {
> ^
> drivers/platform/x86/acer-wmi.c:1018:2: note: here
> default:
> ^~~
>
> Warning level 3 was used: -Wimplicit-fallthrough=3
>
> This patch is part of the ongoing efforts to enable
> -Wimplicit-fallthrough.
>
> Signed-off-by: Gustavo A. R. Silva
> ---
> drivers/platform/x86/acer-wmi.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index fcfeadd1301f..bd87f9037f95 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -1015,6 +1015,7 @@ static acpi_status WMID_get_u32(u32 *value, u32 cap)
> *value = tmp & 0x1;
> return 0;
> }
> + /* fall through */
> default:
> return AE_ERROR;
> }
> @@ -1341,6 +1342,7 @@ static acpi_status get_u32(u32 *value, u32 cap)
> status = AMW0_get_u32(value, cap);
> break;
> }
> + /* fall through */
This doesn't strike me as obviously the right thing to do here. If the interface
type is AMW0_V2, why is it the right thing to do to use WMID_get_u32 if the cap
isn't ACER_CAP_MAILLED?
> case ACER_WMID:
> status = WMID_get_u32(value, cap);
> break;
> @@ -1383,6 +1385,7 @@ static acpi_status set_u32(u32 value, u32 cap)
>
> return AMW0_set_u32(value, cap);
> }
> + /* fall through */
Similarly here.
Are we documenting intended behavior, or covering up a bug.
> case ACER_WMID:
> return WMID_set_u32(value, cap);
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] platform/x86: pmc_atom: Add Lex 3I380D industrial PC to critclk_systems DMI table
On Wed, May 08, 2019 at 11:20:52AM +0200, Hans de Goede wrote: > Hi, > > On 08-05-19 10:42, Andy Shevchenko wrote: > > On Wed, May 8, 2019 at 10:48 AM Hans de Goede wrote: > > > On 07-05-19 22:17, Stephen Boyd wrote: > > > > Quoting Hans de Goede (2019-05-06 08:05:42) > > > > > > I guess this is urgent? > > > > > > Somewhat, getting this into e.g. rc2 would be fine too, waiting till 5.3 > > > would be bad. > > > > So, I can do it as a fixes for rc2, just ping me after merge window. > > Ok, will do. Andy, what is the issue here? If the dependency is in v5.1 we can do a "merge --ff-only v5.1" in our for-next branch in order to pull it in, that would be the same as an immutable branch basically. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] platform/x86: alienware-wmi: fix kfree on potentially uninitialized pointer
On Wed, Apr 03, 2019 at 11:05:12PM +0100, Colin Ian King wrote:
> On 03/04/2019 23:02, Darren Hart wrote:
> > On Sat, Mar 30, 2019 at 12:17:12AM +, Colin King wrote:
> >> From: Colin Ian King
> >>
> >> Currently the kfree of output.pointer can be potentially freeing
> >> an uninitalized pointer in the case where out_data is NULL. Fix this
> >> by reworking the case where out_data is not-null to perform the
> >> ACPI status check and also the kfree of outpoint.pointer in one block
> >> and hence ensuring the pointer is only freed when it has been used.
> >>
> >> Also replace the if (ptr != NULL) idiom with just if (ptr).
> >>
> >> Fixes: ff0e9f26288d ("platform/x86: alienware-wmi: Correct a memory leak")
> >> Signed-off-by: Colin Ian King
> >
> > Thanks for the catch Colin, queued for testing.
> >
> > Did you trigger this error or detect it via review or static analysis?
> >
> Static analysis, I'm now running a licensed version of Coverity on one
> of our servers.
We typically include the tool used to identify such bugs, and I see several such
tags for Coverity in the logs. Was there a reason not to include that tag? If
just an oversight, can you provide that tag and I'll amend the commit.
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] platform/x86: dell-laptop: fix rfkill functionality
On Wed, Apr 03, 2019 at 09:54:27PM +, mario.limoncie...@dell.com wrote:
> > -Original Message-
> > From: Darren Hart
> > Sent: Wednesday, April 3, 2019 3:24 PM
> > To: Limonciello, Mario
> > Cc: Andy Shevchenko; LKML; platform-driver-...@vger.kernel.org
> > Subject: Re: [PATCH] platform/x86: dell-laptop: fix rfkill functionality
> >
> >
> > [EXTERNAL EMAIL]
> >
> > On Wed, Mar 27, 2019 at 09:25:34AM -0500, Mario Limonciello wrote:
> > > When converting the driver two arguments were transposed leading
> > > to rfkill not working.
> > >
> > > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=201427
> > > Reported-by: Pepijn de Vos
> > > Fixes: 549b49 ("platform/x86: dell-smbios: Introduce dispatcher for SMM
> > > calls")
> > > Signed-off-by: Mario Limonciello
> > > Acked-by: Pali Rohár
> >
> > Looks like this broken in 4.14? So Cc stable back to then? I'm surprised
> > it's
> > been broken that long :-(
> >
>
> Not many "newer" platforms actually use this radio control anymore. It's been
> superseded by other technologies. So I think that would be the main reason.
>
> Can you CC it to stable when it's committed or do I need to re-send or
> anything?
>
I will add the Cc to stable. Just wanted to confirm our expectations were
aligned.
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] platform/x86: alienware-wmi: fix kfree on potentially uninitialized pointer
On Sat, Mar 30, 2019 at 12:17:12AM +, Colin King wrote:
> From: Colin Ian King
>
> Currently the kfree of output.pointer can be potentially freeing
> an uninitalized pointer in the case where out_data is NULL. Fix this
> by reworking the case where out_data is not-null to perform the
> ACPI status check and also the kfree of outpoint.pointer in one block
> and hence ensuring the pointer is only freed when it has been used.
>
> Also replace the if (ptr != NULL) idiom with just if (ptr).
>
> Fixes: ff0e9f26288d ("platform/x86: alienware-wmi: Correct a memory leak")
> Signed-off-by: Colin Ian King
Thanks for the catch Colin, queued for testing.
Did you trigger this error or detect it via review or static analysis?
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] platform/x86: dell-laptop: fix rfkill functionality
On Wed, Mar 27, 2019 at 09:25:34AM -0500, Mario Limonciello wrote:
> When converting the driver two arguments were transposed leading
> to rfkill not working.
>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=201427
> Reported-by: Pepijn de Vos
> Fixes: 549b49 ("platform/x86: dell-smbios: Introduce dispatcher for SMM
> calls")
> Signed-off-by: Mario Limonciello
> Acked-by: Pali Rohár
Looks like this broken in 4.14? So Cc stable back to then? I'm surprised it's
been broken that long :-(
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] platform/x86: dell-smbios-base: Fix use after free on failure of dell_smbios_init()
ea000ea31808 8884204113c0 > raw: 000d000d 0001 > page dumped because: kasan: bad access detected > > Memory state around the buggy address: > 88840c2bc080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc > 88840c2bc100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc > >88840c2bc180: fc fc fc fc fc fb fb fb fb fb fb fb fb fb fb fb > ^ > 88840c2bc200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > 88840c2bc280: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > == > > Link: http://lkml.kernel.org/r/1553106560.2080.5.ca...@gmail.com > > Reported-by: Tom Zanussi > Tested-by: Tom Zanussi > Signed-off-by: Steven Rostedt (VMware) > --- > drivers/platform/x86/dell-smbios-base.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/platform/x86/dell-smbios-base.c > b/drivers/platform/x86/dell-smbios-base.c > index 0537d44d45a6..a74c0df25b15 100644 > --- a/drivers/platform/x86/dell-smbios-base.c > +++ b/drivers/platform/x86/dell-smbios-base.c > @@ -625,6 +625,8 @@ static int __init dell_smbios_init(void) > > fail_platform_driver: > kfree(da_tokens); > + da_tokens = NULL; > + da_num_tokens = 0; > return ret; > } > > -- > 2.20.1 > > -- Darren Hart VMware Open Source Technology Center
Re: [GIT PULL] platform-drivers-x86 for 5.1-1
On Sun, Mar 10, 2019 at 01:22:24PM -0700, Linus Torvalds wrote: > On Fri, Mar 8, 2019 at 12:33 PM Darren Hart wrote: > > > > The wmi mod alias changes collide with a similar series for TEE based > > devices. > > Anyway, you also used a *very* new subkey. Can you please mention it, > so that I don't then get taken by surprise when I find a key that was > used to sign this and I don't have it? > > Yes, yes, I will download it anyway, but basically I want the pull > request to give me enough information that when I do the pull I'm not > surprised by the result. > > That's why the shortlog and diffstat exists of course, but that "don't > surprise Linus" also covers things like "oh, and the tag will be > signed with a key you've never seen before". Will do. Apologies. I almost sent you a note - but didn't after verifying with others that git-verify-tag still reported "Good Signature" as the subkey is automatically associated with the master key - but, point taken - don't surprise Linus. Will err on the side of more explanation in the future. Thanks, -- Darren Hart VMware Open Source Technology Center
[GIT PULL] platform-drivers-x86 for 5.1-1
Hi Linus, The wmi mod alias changes collide with a similar series for TEE based devices. I have provided my merge resolution here: git://git.infradead.org/linux-platform-drivers-x86.git pdx86-5.1-1-merge This series includes two previously merged patches from the RC cycle: - 522e4ee6e5 Documentation/ABI: Correct mlxreg-io KernelVersion for 5.0 - ff7c634b4f x86/CPU: Add Icelake model number Thanks, Darren Hart VMware Open Source Technology Center The following changes since commit 49a57857aeea06ca831043acbb0fa5e0f50602fd: Linux 5.0-rc3 (2019-01-21 13:14:44 +1300) are available in the Git repository at: git://git.infradead.org/linux-platform-drivers-x86.git tags/platform-drivers-x86-v5.1-1 for you to fetch changes up to 9c22cc020db637850ba47a14a598d09f706f19ad: MAINTAINERS: Include mlxreg.h in Mellanox Platform Driver files (2019-03-07 08:46:29 -0800) platform-drivers-x86 for v5.1-1 Use MODULE_DEVICE_TABLE across several wmi drivers, keeping wmi_device_id and MODULE_ALIAS() declarations in sync. Add several Ideapad models to the no_hw_rfkill list. Add support for new Mellanox platforms, including new fan and LED functionality. Address Dell keyboard backlight change event and power button release issues. Update dell_rbu to use appropriate memory allocation mechanisms. Several small fixes and Ice Lake support for intel_pmc_core. Fix a suspend regression for Cherry Trail based devices in intel_int0002_vgpio. A few other routine fixes. The following is an automated git shortlog grouped by driver: ACPI / scan: - Create platform device for BSG2150 ACPI nodes Documentation/ABI: - Add new attribute for mlxreg-io sysfs interfaces - Correct mlxreg-io KernelVersion for 5.0 MAINTAINERS: - Include mlxreg.h in Mellanox Platform Driver files asus-wmi: - Allow loading on systems without the Asus Management GUID dell-smbios-wmi: - use MODULE_DEVICE_TABLE() instead of MODULE_ALIAS() dell-wmi: - use MODULE_DEVICE_TABLE() instead of MODULE_ALIAS() - Ignore new keyboard backlight change event dell-wmi-descriptor: - use MODULE_DEVICE_TABLE() instead of MODULE_ALIAS() dell_rbu: - fix lock imbalance in img_update_realloc - stop abusing the DMA API huawei-wmi: - use MODULE_DEVICE_TABLE() instead of MODULE_ALIAS() ideapad-laptop: - Add ideapad 330-15ICH to no_hw_rfkill - Add S130-14IGM to no_hw_rfkill list - Add Ideapad 530S-14ARR to no_hw_rfkill list - Add Yoga C930 to no_hw_rfkill_list - Add Y530-I5ICH-1060 to no_hw_rfkill list - Fix no_hw_rfkill_list for Lenovo RESCUER R720-15IKBN intel-hid: - Missing power button release on some Dell models intel-wmi-thunderbolt: - use MODULE_DEVICE_TABLE() instead of MODULE_ALIAS() intel_int0002_vgpio: - Only implement irq_set_wake on Bay Trail intel_pmc_core: - Quirk to ignore XTAL shutdown - Add Package cstates residency info - Add ICL platform support - Convert to INTEL_CPU_FAM6 macro - Avoid a u32 overflow - Include Reserved IP for LTR - Fix file permissions for ltr_show - Fix PCH IP name - Fix PCH IP sts reading - Handle CFL regmap properly leds: - mlxreg: Add support for capability register mlx-platform: - Fix access mode for fan_dir attribute - Add UID LED for the next generation systems - Add extra CPLD for next generation systems - Add support for new VMOD0007 board name - Add support for fan capability registers - Add support for fan direction register modpost: - file2alias: define size of alias platform/mellanox: - mlxreg-hotplug: Fix KASAN warning platform_data/mlxreg: - Add capability field to core platform data - Document fixes for core platform data touchscreen_dmi: - Add info for the CHUWI Hi10 Air tablet - Add info for the Chuwi Hi8 Air tablet - Add info for the PoV Wintab P1006w (v1.0) tablet wmi: - add WMI support to MODULE_DEVICE_TABLE() - move struct wmi_device_id to mod_devicetable.h - fix potential null pointer dereference wmi-bmof: - use MODULE_DEVICE_TABLE() instead of MODULE_ALIAS() x86/CPU: - Add Icelake model number Anthony Wong (1): platform/x86: ideapad: Add ideapad 330-15ICH to no_hw_rfkill Christian Oder (1): platform/x86: touchscreen_dmi: Add info for the CHUWI Hi10 Air tablet Christoph Hellwig (2): platform/x86: dell_rbu: stop abusing the DMA API platform/x86: dell_rbu: fix lock imbalance in img_update_realloc Darren Hart (VMware) (2): Documentation/ABI: Correct mlxreg-io KernelVersion for 5.0 MAINTAINERS: Include mlxreg.h in Mellanox Platform Driver files Felix Eckhofer (1): platform/x86: ideapad-laptop: Add S130-14IGM to no_hw_rfkill list Hans de Goede (4): ACPI / scan: Create platform device for BSG2150 ACPI nodes platform/x86: touchscreen_dmi: Add info for the PoV Wintab P1006w (v1.0) tablet platform/x86
Re: [PATCH 2/3] x86: apuv2: fix input dependencies
On Thu, Mar 07, 2019 at 01:10:13AM +0100, Enrico Weigelt, metux IT consult wrote: > On 05.03.19 14:56, Andy Shevchenko wrote: > > > > Darren gave a talk about merging kernel configs to get something like > > you want to. > > This tool is quite long already lying around. merge_config.sh in your > > kernel source tree. > > Yes, that's similar to how some distros (eg. yocto) do it. > I wrote merge_config.sh to replace and simplify some of the yocto tooling. With merge_config upstream, Yocto now uses it directly. > But my requirements are a bit more complex: > > In my final meta-config, I just wanna say: > > * i have board A (possibly multiple boards) > * i need features X, Y, Z (eg. eth, display, can, ext4, acl, ...) > > And that shall be all to generate a minimal config for exactly those > requirements. That's also the goal of the Yocto configuration fragments, and is possible with merge_config with a set of defined fragments. > > Doing that by just putting config snippets together, quickly turns into > a maintenance hell. At least you'd need recursive dependencies and some > if/else logic. > > That's why I've written kmct: > > https://github.com/metux/kmct I had a look, the README could benefit from a basic usage example. Digging through it further, it appears that you are creating yaml files which contain CONFIGs. The problem with this in my opinion is these are kernel version specific, so you know have a lot of boiler plate yaml wrapping kernel version specific CONFIG options which will slowly get out of sync over time. This is the usual argument for keeping config fragments together with the kernel - and why we do that in arch/*/config for example. Perhaps I'm missing your desired workflow though. I tend to find the options I need and record them in a fragment, and save it off for later to quickly be able to "make defconfig fragA.config fragB.config" etc. Is what you're trying to do different? -- Darren Hart VMware Open Source Technology Center
Re: [PATCH v4 0/8] platform/x86: wmi: add WMI support to
On Tue, Feb 19, 2019 at 08:59:48PM +0100, Mattias Jacobsson wrote: > The kernel provides the macro MODULE_DEVICE_TABLE() which can help > driver authors to generate the appropriate MODULE_ALIAS() output. The > WMI device type is currently not supported by MODULE_DEVICE_TABLE(). Thanks Mattias. I have this queued up for testing, and with an ack from Masahiro for 2/8, I'll pull this in. Thanks for the patches. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] modpost: file2alias: define size of alias
On Wed, Mar 06, 2019 at 10:09:26PM -0800, Darren Hart wrote: > On Sat, Feb 23, 2019 at 08:57:50PM +0100, Mattias Jacobsson wrote: > > On 2019-02-20, Masahiro Yamada wrote: > > > > > > If you want all the patches to go through x86 platform-driver tree, > > > > > I am fine with that too. > > > > > > > > I don't mind either way, however I've asked the x86 platform-driver > > > > maintainers if they have a preference in this matter. You should have > > > > received that mail otherwise see [1]. > > > > > > > > [1]: > > > > https://lkml.kernel.org/r/082d3d38b7dde6050b6b3e518ada439eade65b2c.1550603967.git@mok.nu > > > > > > > > > I saw it. The 2/8 uses ALIAS_SIZE. > > > > > > So, I think it will be better to include this one in your series. > > > If necessary, please feel free to add > > > > > > Acked-by: Masahiro Yamada > > > > > > > Okey, will do. Thanks! > > > Agree, I'll pull it in. Masahiro, may I include your Acked-by on the 2/8 patch as well? platform/x86: wmi: add WMI support to MODULE_DEVICE_TABLE() -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] modpost: file2alias: define size of alias
On Sat, Feb 23, 2019 at 08:57:50PM +0100, Mattias Jacobsson wrote: > On 2019-02-20, Masahiro Yamada wrote: > > > > If you want all the patches to go through x86 platform-driver tree, > > > > I am fine with that too. > > > > > > I don't mind either way, however I've asked the x86 platform-driver > > > maintainers if they have a preference in this matter. You should have > > > received that mail otherwise see [1]. > > > > > > [1]: > > > https://lkml.kernel.org/r/082d3d38b7dde6050b6b3e518ada439eade65b2c.1550603967.git@mok.nu > > > > > > I saw it. The 2/8 uses ALIAS_SIZE. > > > > So, I think it will be better to include this one in your series. > > If necessary, please feel free to add > > > > Acked-by: Masahiro Yamada > > > > Okey, will do. Thanks! Agree, I'll pull it in. -- Darren Hart VMware Open Source Technology Center
[PATCH] MAINTAINERS: Include mlxreg.h in Mellanox Platform Driver files
Avoid conflicts from other subsystems by including the header with the rest of the driver files. Cc: Andy Shevchenko Cc: Vadim Pasternak Signed-off-by: Darren Hart (VMware) --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 51029a425dbe..eb0b9aba4802 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9711,6 +9711,7 @@ M:Vadim Pasternak L: platform-driver-...@vger.kernel.org S: Supported F: drivers/platform/mellanox/ +F: include/linux/platform_data/mlxreg.h MELLANOX MLX4 core VPI driver M: Tariq Toukan -- 2.17.2 -- Darren Hart VMware Open Source Technology Center
Re: linux-next: manual merge of the drivers-x86 tree with the watchdog tree
On Wed, Mar 06, 2019 at 09:27:01PM -0800, Darren Hart wrote:
> On Mon, Mar 04, 2019 at 02:37:35PM +1100, Stephen Rothwell wrote:
> > Hi all,
> >
> > Today's linux-next merge of the drivers-x86 tree got a conflict in:
> >
> > include/linux/platform_data/mlxreg.h
> >
> > between commit:
> >
> > 9f03161a1bd8 ("platform_data/mlxreg: additions for Mellanox watchdog
> > driver.")
> >
> > from the watchdog tree and commit:
> >
> > 9b28aa1d0eae ("platform_data/mlxreg: Document fixes for core platform
> > data")
> >
> > from the drivers-x86 tree.
> >
> > I fixed it up (see below) and can carry the fix as necessary. This
> > is now fixed as far as linux-next is concerned, but any non trivial
> > conflicts should be mentioned to your upstream maintainer when your tree
> > is submitted for merging. You may also want to consider cooperating
> > with the maintainer of the conflicting tree to minimise any particularly
> > complex conflicts.
>
> Thanks Stephen.
>
> I suspect this will be a rare occurence. That said - Vadim - could you please
> ensure that all the mellanox driver changes Cc the platform driver x86 mailing
> list by adding it to MAINTAINERS?
And, turns out, not so rare. We've been down this road before. I'll just prepare
the patch for the MAINTAINERS.
--
Darren Hart
VMware Open Source Technology Center
Re: linux-next: manual merge of the drivers-x86 tree with the watchdog tree
On Mon, Mar 04, 2019 at 02:37:35PM +1100, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the drivers-x86 tree got a conflict in:
>
> include/linux/platform_data/mlxreg.h
>
> between commit:
>
> 9f03161a1bd8 ("platform_data/mlxreg: additions for Mellanox watchdog
> driver.")
>
> from the watchdog tree and commit:
>
> 9b28aa1d0eae ("platform_data/mlxreg: Document fixes for core platform data")
>
> from the drivers-x86 tree.
>
> I fixed it up (see below) and can carry the fix as necessary. This
> is now fixed as far as linux-next is concerned, but any non trivial
> conflicts should be mentioned to your upstream maintainer when your tree
> is submitted for merging. You may also want to consider cooperating
> with the maintainer of the conflicting tree to minimise any particularly
> complex conflicts.
Thanks Stephen.
I suspect this will be a rare occurence. That said - Vadim - could you please
ensure that all the mellanox driver changes Cc the platform driver x86 mailing
list by adding it to MAINTAINERS?
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH v5 2/7] x86, olpc: Use a correct version when making up a battery node
On Thu, Jan 10, 2019 at 06:40:00PM +0100, Lubomir Rintel wrote: > The XO-1 and XO-1.5 batteries apparently differ in an ability to report > ambient temperature. Add a different compatible string to the 1.5 > battery. Hi Lubomir, Thanks for the recent Cc and pointer to here. In general, I have no problem with the net changes. I do have some concerns from a reviewability and change documentation perspective. You document your intent above, but you also made several other changes to get there which aren't documented, so when reviewing they stand out as "why is this here?". >From what I can tell, this change contains: 1) Cleanup of olpc_dt_interpret() calls to avoid splitting string literals (noted in the patch history, but not called out as an intentional change) 2) Refactoring of logic and breaking out the check for the compatible property into olpc_dt_compatible_match 3) Addition of "we're running very old firmware if this is missing" checks - I'm not sure how this relates to the stated problem and the intended fix. 4) The addition of the xo1.5 compatible property. All the other things makes it very difficult to determine if this patch does what you describe - and only what you describe. In general please: a) Cleanup code b) Refactor code c) Change functionality In that order - that way the new functionality is what show up when someone does a git blame on the file (rather than a cleanup patch, which isn't as useful in defect analysis). And always document in your commit messages the approach you take to fix the reported issue. Thanks, -- Darren Hart VMware Open Source Technology Center
Re: linux-next: Signed-off-by missing for commit in the drivers-x86 tree
On Sun, Feb 24, 2019 at 01:19:25AM +1100, Stephen Rothwell wrote:
> Hi all,
>
> Commit
>
> 9bfe33ba29d0 ("x86/CPU: Add Icelake model number")
>
> is missing a Signed-off-by from its committer.
>
Hi Stephen,
Apologies if I've asked you this before... I didn't find it after some
searching.
We should be catching errors like this before they hit next. First,
there is no reason we can't catch them - unlike the integration failures
only next can catch. Second, once they are in next, there is no "right"
way to fix them. Either rebase or send the bad patch to mainline - both
are bad. Don't get me wrong, I'm glad next catches them but I'd like
to get to the point where it doesn't trigger on our subsystem :-)
Are your patch mechanics tests available for us to integrate into our
commit and prepublication checks?
Thanks,
--
Darren Hart
VMware Open Source Technology Center
Re: linux-next: Signed-off-by missing for commit in the drivers-x86 tree
On Sun, Feb 24, 2019 at 01:19:25AM +1100, Stephen Rothwell wrote:
> Hi all,
>
> Commit
>
> 9bfe33ba29d0 ("x86/CPU: Add Icelake model number")
>
> is missing a Signed-off-by from its committer.
Thanks for the catch. Andy, please check your commit scripts as should
definitely be automated in our workflow.
I have corrected it.
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH v2] platform/x86: wmi: fix potential null pointer dereference
On Wed, Jan 30, 2019 at 05:43:34PM +0200, Andy Shevchenko wrote: > On Wed, Jan 30, 2019 at 5:15 PM Mattias Jacobsson <2...@mok.nu> wrote: > > > > In the function wmi_dev_match() the variable id is dereferenced without > > first performing a NULL check. The variable can for example be NULL if > > a WMI driver is registered without specifying the id_table field in > > struct wmi_driver. > > > > Add a NULL check and return that the driver can't handle the device if > > the variable is NULL. > > > > Good for me, thanks! Queued for testing, thanks Mattias. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH v5 06/10] Platform: OLPC: Add XO-1.75 EC driver
On Thu, Jan 10, 2019 at 06:58:41PM +0100, Lubomir Rintel wrote: > It's based off the driver from the OLPC kernel sources. Somewhat > modernized and cleaned up, for better or worse. > > Modified to plug into the olpc-ec driver infrastructure (so that battery > interface and debugfs could be reused) and the SPI slave framework. > > Signed-off-by: Lubomir Rintel > > --- > Changes since v4: > - Chop off the reboot handler. Will be added back once it's clear how. > > Changes since v3: > - Introduce CONFIG_OLPC_EC symbol to enable parts common to this driver > and the X86 OLPC EC machinery. > > Changes since v1: > - Cosmetic style changes; whitespace, ordering of declarations and > #includes, remoted extra comas from sentinels > - Count the terminating NUL in LOG_BUF_SIZE > - Make olpc_xo175_ec_is_valid_cmd() return -EINVAL instead of -1 > on error > - Spell keyboard/touchpad in full for CHAN_KEYBOARD/TOUCHPAD messages > - Use a #define for PM wakeup processing time > - Log a message on unknown event > - Escape logging payload with %*pE > - Replace an open-coded min() > - Correct an error code on short read > - replaced PM callback #ifdefs with __maybe_unusedm SET_RUNTIME_PM_OPS > and SET_NOIRQ_SYSTEM_SLEEP_PM_OPS > - dev_get_drvdata() instead of a round-trip through platform device > - s/unsigned char x/u8 x/ in olpc_xo175_ec_resume() > - Use GENMASK() instead of 0x for the event mask > - Replace cmd tx/resp rx buffers with structures > - Turned suspend hint arguments into a struct, and tidied up the comment > > Basically all of the above is based on the review by Andy Shevchenko. Andy, I am happy to merge this series - but this patch includes a lot of changes driven by your feedback. Are you satisfied with the changes, would you like to add your Reviewed-by? -- Darren Hart VMware Open Source Technology Center
Re: [PATCH v5 01/10] dt-bindings: olpc,xo1.75-ec: Add OLPC XO-1.75 EC bindings
On Fri, Jan 11, 2019 at 08:36:35AM -0600, Rob Herring wrote: > On Thu, 10 Jan 2019 18:58:36 +0100, Lubomir Rintel wrote: > > The OLPC XO-1.75 Embedded Controller is a SPI master that uses extra > > signals for handshaking. It needs to know when is the slave (Linux) > > side's TX FIFO ready for transfer (the ready-gpio signal on the SPI > > controller node) and when does it wish to respond with a command (the > > cmd-gpio property). > > > > Signed-off-by: Lubomir Rintel > > Acked-by: Pavel Machek > > > > --- > > Changes since v1: > > - s/cmd-gpio/cmd-gpios/ > > - s/ready-gpio/ready-gpios/ in the documentation paragraph > > - Remove status = "okay" from the example > > > > .../bindings/misc/olpc,xo1.75-ec.txt | 23 +++ > > 1 file changed, 23 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/misc/olpc,xo1.75-ec.txt > > > > Please add Acked-by/Reviewed-by tags when posting new versions. However, > there's no need to repost patches *only* to add the tags. The upstream > maintainer will do that for acks received on the version they apply. > > If a tag was not added on purpose, please state why and what changed. Rob, I saw you ack this patch in an earlier version. Based on your comment here, is that ack still valid? -- Darren Hart VMware Open Source Technology Center
Re: linux-next: Fixes tag needs some work in the drivers-x86 tree
On Wed, Feb 13, 2019 at 08:45:56PM +0530, Bhardwaj, Rajneesh wrote:
>
> On 07-Feb-19 9:25 PM, Andy Shevchenko wrote:
> > On Thu, Feb 7, 2019 at 4:06 AM Bhardwaj, Rajneesh
> > wrote:
> > > On 07-Feb-19 4:27 AM, Stephen Rothwell wrote:
> > >
> > > Hi all,
> > >
> > > In commit
> > >
> > >4284dc008f43 ("platform/x86: intel_pmc_core: Fix file permissions for
> > > ltr_show")
> > >
> > > Fixes tag
> > >
> > >Fixes: 63cde0c16c67 ("platform/x86: intel_pmc_core: Show Latency
> > > Tolerance info")
> > >
> > > has these problem(s):
> > >
> > >- Target SHA1 does not exist
> > >
> > > Did you mean:
> > >
> > >2eb150558bb7 ("platform/x86: intel_pmc_core: Show Latency Tolerance
> > > info")
> > >
> > > Yes, upstream commit is 2eb150558bb79ee01c39b64c2868216c0be2904f. For
> > > some reason when i do git show on my repo with both these SHA1 i see the
> > > same patch.
> > >
> > > I will fix this in next version.
> > Hmm... this came to our published branch, i.e. for-next, would it be
> > better to update it via rebasing?
> >
> > Darren, what do you think?
>
> Hi Andy, I have corrected this in v2 anyway and i sent to upstream today,
> just in case you prefer it over rebasing.
>
> https://patchwork.kernel.org/patch/10810123/
>
While we try hard not to rebase, if the choice is to rebase for-next or send a
bad commit to upstream, I will opt for the rebase.
Andy, I would suggest doing the rebase.
--
Darren Hart
VMware Open Source Technology Center
[GIT PULL] platform-drivers-x86 for 5.0-2
Hi Linus, The following changes since commit 49a57857aeea06ca831043acbb0fa5e0f50602fd: Linux 5.0-rc3 (2019-01-21 13:14:44 +1300) are available in the Git repository at: git://git.infradead.org/linux-platform-drivers-x86.git tags/platform-drivers-x86-v5.0-2 for you to fetch changes up to 6a730fcb9cb472ba2d42b26a50ac65dacdd68882: Documentation/ABI: Correct mlxreg-io KernelVersion for 5.0 (2019-01-26 11:30:26 -0800) Thanks, Darren Hart VMware Open Source Technology Center platform-drivers-x86 for v5.0-2 Correct Documentation/ABI 4.21 KernelVersion to 5.0. The following is an automated git shortlog grouped by driver: Documentation/ABI: - Correct mlxreg-io KernelVersion for 5.0 Darren Hart (VMware) (1): Documentation/ABI: Correct mlxreg-io KernelVersion for 5.0 Documentation/ABI/stable/sysfs-driver-mlxreg-io | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] dell_rbu: stop abusing the DMA API
On Sat, Feb 02, 2019 at 06:16:59PM +0100, Christoph Hellwig wrote: > On Fri, Feb 01, 2019 at 03:15:59PM -0800, Darren Hart wrote: > > On Tue, Jan 29, 2019 at 08:34:09AM +0100, Christoph Hellwig wrote: > > > For some odd reason dell_rbu actually seems to want the physical and > > > not a bus address for the allocated buffer. Lets assume that actually > > > is correct given that it is BIOS-related and that is a good source > > > of insanity. In that case we should not use dma_alloc_coherent with > > > a NULL device to allocate memory, but use GFP_DMA32 to stay under > > > the 32-bit BIOS limit. > > > > + Mario re bios related physical address - is Christoph's assumption > > correct? > > > > Christoph, did you observe a failure? If so, we should probably also > > tag for stable. > > No, I've been auditing for DMA API (ab-)users that don't pass a > struct device. Generally the fix was to just pass a struct device > that is easily available. But dell_rbu doesn't actually seem to > be a "device" in the traditional sense, and the way it uses the > DMA API is really, really odd - it first does a virt_to_phys on > memory allocated from the page allocator (so works with physical > addresses in that case) and the retries with a dma_alloc_coherent > with a NULL argument, which in no way is guaranteed to you give > you something else, although for the current x86 implementation > will give you the equivalent of a GFP_DMA32 page allocator allocation > plus virt_to_phys. > Thanks Christoph, merged to for-next. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] dell_rbu: stop abusing the DMA API
On Tue, Jan 29, 2019 at 08:34:09AM +0100, Christoph Hellwig wrote: > For some odd reason dell_rbu actually seems to want the physical and > not a bus address for the allocated buffer. Lets assume that actually > is correct given that it is BIOS-related and that is a good source > of insanity. In that case we should not use dma_alloc_coherent with > a NULL device to allocate memory, but use GFP_DMA32 to stay under > the 32-bit BIOS limit. + Mario re bios related physical address - is Christoph's assumption correct? Christoph, did you observe a failure? If so, we should probably also tag for stable. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] dell_rbu: stop abusing the DMA API
On Fri, Feb 01, 2019 at 12:28:46PM -0600, Stuart Hayes wrote: > > On 1/29/2019 1:34 AM, Christoph Hellwig wrote: > > For some odd reason dell_rbu actually seems to want the physical and > > not a bus address for the allocated buffer. Lets assume that actually > > is correct given that it is BIOS-related and that is a good source > > of insanity. In that case we should not use dma_alloc_coherent with > > a NULL device to allocate memory, but use GFP_DMA32 to stay under > > the 32-bit BIOS limit. > > > > Signed-off-by: Christoph Hellwig > > --- ... > > Acked-by: Stuart Hayes > Ah, prematurely pulled in Mario - thanks Stuart. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] MAINTAINERS: Add Andy and Darren as arch/x86/platform/ reviewers
On Mon, Jan 28, 2019 at 12:36:19PM +0100, Borislav Petkov wrote: > From: Borislav Petkov > > ... so that they can get CCed on platform patches. > > Signed-off-by: Borislav Petkov > Cc: Darren Hart > Cc: Andy Shevchenko > Cc: x...@kernel.org Acked-by: Darren Hart (VMware) -- Darren Hart VMware Open Source Technology Center
Re: [PATCH v2] platform/x86: ideapad-laptop: Fix no_hw_rfkill_list for Lenovo RESCUER R720-15IKBN
On Sat, Jan 19, 2019 at 07:16:33PM +0800, Yang Fan wrote:
> Commit ae7c8cba3221 ("platform/x86: ideapad-laptop: add lenovo RESCUER
> R720-15IKBN to no_hw_rfkill_list") added
> DMI_MATCH(DMI_BOARD_NAME, "80WW")
> for Lenovo RESCUER R720-15IKBN.
>
> But DMI_BOARD_NAME does not match 80WW on Lenovo RESCUER R720-15IKBN,
> thus cause Wireless LAN still be hard blocked.
>
> On Lenovo RESCUER R720-15IKBN:
> ~$ cat /sys/class/dmi/id/sys_vendor
> LENOVO
> ~$ cat /sys/class/dmi/id/board_name
> Provence-5R3
> ~$ cat /sys/class/dmi/id/product_name
> 80WW
> ~$ cat /sys/class/dmi/id/product_version
> Lenovo R720-15IKBN
>
> So on Lenovo RESCUER R720-15IKBN:
> DMI_SYS_VENDOR should match "LENOVO",
> DMI_BOARD_NAME should match "Provence-5R3",
> DMI_PRODUCT_NAME should match "80WW",
> DMI_PRODUCT_VERSION should match "Lenovo R720-15IKBN".
>
> Fix it, and in according with other entries in no_hw_rfkill_list,
> use DMI_PRODUCT_VERSION instead of DMI_BOARD_NAME.
>
> Fixes: ae7c8cba3221 ("platform/x86: ideapad-laptop: add lenovo
> RESCUER R720-15IKBN to no_hw_rfkill_list")
> Signed-off-by: Yang Fan
Thanks for the patch, queued for testing.
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] platform/x86: asus-wmi: Allow loading on systems without the Asus Management GUID
On Mon, Jan 21, 2019 at 02:24:36PM +0100, Hans de Goede wrote: > hid-asus depends on asus-wmi through the asus_wmi_evaluate_method. Before > this commit asus-wmi and thus hid-asus could not be loaded on non Asus > systems. This breaks using Asus bluetooth keyboards such as the Asus > T100CHI keyboard with non Asus systems. > > This commit fixes this by allowing asus-wmi to load on systems without the > Asus Management GUID. > > This is save to do since all asus-wmi sub drivers use > asus_wmi_register_driver which also checks for the GUID. > > This commit also improves the error messages in asus_wmi_register_driver > to include "ASUS" in their description tom make them more clear. This is > important since we now rely on those errors when loaded on systems without > the Asus Management GUID. > > Signed-off-by: Hans de Goede Hi Hans, A few typos above, please consider adding a spellchecker to your editor for commit message authoring... I've taken care of them and queued this patch for testing. Thanks! -- Darren Hart VMware Open Source Technology Center
Re: [PATCH 0/3] platform/x86: wmi: add WMI support to MODULE_DEVICE_TABLE()
On Sat, Jan 19, 2019 at 12:55:52PM +0100, Mattias Jacobsson wrote: > This patchset adds WMI support to MODULE_DEVICE_TABLE(). Hi Mattias, Thanks for the patch series. I've reviewed and found no major issues - but what I'm missing from this cover letter and each commit message is the why. What is the problem this series is intended to address? This should be clear in the commit messages so developers reading the git history have the necessary context to understand why the change was made and the intent behind them were. The only advantage that I see by the end of the series is removing the need for driver authors to prefix the modules alias with "wmi:" - which is an advantage and avoids careless errors. Is that the only goal? It adds some complexity by spreading the implementation across more files and more directories, so we need to document the justification. Thanks, -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] platform/x86: wmi: fix potential null pointer dereferences
On Tue, Jan 22, 2019 at 09:03:01PM +0100, Mattias Jacobsson wrote: > In the function wmi_dev_match() there are three variables that > potentially can result in a null pointer dereference. Namely: Is this something you have observed? This gets called when a new driver registered for each unassociated device on the bus, so I'm not immediately seeing how dev or driver would end up being NULL here. See: Documentation/driver-model/bus.txt -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] futex: no need to check return value of debugfs_create functions
On Tue, Jan 22, 2019 at 04:21:39PM +0100, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value. The function can work or not, but the code logic should
> never do something different based on this.
>
> Cc: Thomas Gleixner
> Cc: Ingo Molnar
> Cc: Peter Zijlstra
> Cc: Darren Hart
> Signed-off-by: Greg Kroah-Hartman
As documented, and futexes can't be modules, so no need to check for the dentry.
Reviewed-by: Darren Hart (VMware)
> ---
> kernel/futex.c | 8 ++--
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/futex.c b/kernel/futex.c
> index be3bff2315ff..c2ed57b084a4 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -321,12 +321,8 @@ static int __init fail_futex_debugfs(void)
> if (IS_ERR(dir))
> return PTR_ERR(dir);
>
> - if (!debugfs_create_bool("ignore-private", mode, dir,
> - _futex.ignore_private)) {
> - debugfs_remove_recursive(dir);
> - return -ENOMEM;
> - }
> -
> + debugfs_create_bool("ignore-private", mode, dir,
> + _futex.ignore_private);
> return 0;
> }
>
> --
> 2.20.1
>
>
--
Darren Hart
VMware Open Source Technology Center
[PATCH] Documentation/ABI: Correct mlxreg-io KernelVersion for 5.0
The mlxreg-io for the merge window assumed 4.21 as the next kernel version. Replace 4.21 with 5.0. Signed-off-by: Darren Hart (VMware) --- Documentation/ABI/stable/sysfs-driver-mlxreg-io | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/ABI/stable/sysfs-driver-mlxreg-io b/Documentation/ABI/stable/sysfs-driver-mlxreg-io index 9b642669cb16..169fe08a649b 100644 --- a/Documentation/ABI/stable/sysfs-driver-mlxreg-io +++ b/Documentation/ABI/stable/sysfs-driver-mlxreg-io @@ -24,7 +24,7 @@ What: /sys/devices/platform/mlxplat/mlxreg-io/hwmon/hwmon*/ cpld3_version Date: November 2018 -KernelVersion: 4.21 +KernelVersion: 5.0 Contact: Vadim Pasternak Description: These files show with which CPLD versions have been burned on LED board. @@ -35,7 +35,7 @@ What: /sys/devices/platform/mlxplat/mlxreg-io/hwmon/hwmon*/ jtag_enable Date: November 2018 -KernelVersion: 4.21 +KernelVersion: 5.0 Contact: Vadim Pasternak Description: These files enable and disable the access to the JTAG domain. By default access to the JTAG domain is disabled. @@ -105,7 +105,7 @@ What: /sys/devices/platform/mlxplat/mlxreg-io/hwmon/hwmon*/ reset_voltmon_upgrade_fail Date: November 2018 -KernelVersion: 4.21 +KernelVersion: 5.0 Contact: Vadim Pasternak Description: These files show the system reset cause, as following: ComEx power fail, reset from ComEx, system platform reset, reset -- 2.17.2 -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] platform/x86: intel-hid: Missing power button release on some Dell models
On Mon, Jan 07, 2019 at 03:36:48PM +, mario.limoncie...@dell.com wrote: > > > > -Original Message- > > From: platform-driver-x86-ow...@vger.kernel.org > ow...@vger.kernel.org> On Behalf Of Jérôme de Bretagne > > Sent: Sunday, January 6, 2019 11:57 AM > > To: Alex Hung > > Cc: platform-driver-...@vger.kernel.org; Andy Shevchenko; Darren Hart; > > Limonciello, Mario; Rafael J. Wysocki; Chih-Wei Huang; Tristian Celestin; > > linux- > > ker...@vger.kernel.org > > Subject: [PATCH] platform/x86: intel-hid: Missing power button release on > > some > > Dell models > > > > > > [EXTERNAL EMAIL] > > > > Power button suspend for some Dell models was added in: > > > > commit 821b85366284 (intel-hid: Power button suspend on Dell Latitude 7275) I've addressed this one, but please do run checkpatch and correct reported errors in the future. It checks commit reference format. > > > > by checking against the power button press notification (0xCE) to report > > the power button press event. The corresponding power button release > > notification (0xCF) was caught and ignored to stop it from being reported > > as an "unknown event" in the logs. > > > > The missing button release event is creating issues on Android-x86, as > > reported on the project mailing list for a Dell Latitude 5175 model, since > > the events are expected in down/up pairs. > > > > Report the power button release event to fix this issue. > > > > Link: https://groups.google.com/forum/#!topic/android-x86/aSwZK9Nf9Ro > > Tested-by: Tristian Celestin > > Tested-by: Jérôme de Bretagne > > Signed-off-by: Jérôme de Bretagne Queued for testing, thanks for the patch. -- Darren Hart VMware Open Source Technology Center
Re: Keyboard backlight not working on my NP900X5N laptop
On Fri, Jan 04, 2019 at 05:16:38PM +0100, Jan Vlietland wrote:
> Hi Darren,
>
> I understand your extra workload. For me it is just being another user
> complaining about some bug. Sorry for that :-) Good to know the response
> time. I will keep that in mind.
>
> Anyway. I have changed the static variable to "0A", recompiled the module
> and I get the same output 'no such device'.
>
> However I am now running in EFI mode based on another bug:
>
> https://bugs.freedesktop.org/show_bug.cgi?id=109209
>
> ...and I see in the code
>
> struct samsung_laptop *samsung;
> int ret;
>
> if (efi_enabled(EFI_BOOT))
> return -ENODEV;
>
> quirks = _unknown;
> if (!force && !dmi_check_system(samsung_dmi_table))
> return -ENODEV;
>
> samsung = kzalloc(sizeof(*samsung), GFP_KERNEL);
> if (!samsung)
> return -ENOMEM
>
> Is that EFI restriction still valid. As far as I remember Samsung repaired
> their BIOS. Or does the driver not work in EFI mode anyway?
Hi Jan,
Taking a closer look at the driver and the git log, the driver was disabled for
EFI because it pokes at BIOS memory and that would mean poking at a completely
different memory map when in EFI mode - it would be expected to fail - and in
some cases it failed by bricking the laptop. See:
e0094244e41c samsung-laptop: Disable on EFI hardware
What appears to be needed here is for someone with the hardware and some
experience tracing ACPI calls from whatever OS it ships with to figure out the
new interface. I suspect it is either pure ACPI or possibly WMI, and a new
driver may be needed.
Have you tried this driver in BIOS mode with the OA change above?
>
>
> On 03-01-19 03:03, Darren Hart wrote:
> > On Mon, Dec 31, 2018 at 08:40:43PM +0100, Jan Vlietland wrote:
> > > Hi all,
> > >
> > Hey Jan,
> >
> > > Greg K-H suggested to mail you guys.
> > >
> > > I installed Linux 4.20.0-rc7 (downloaded, compiled and installed) on a
> > > Samsung NP900X5N laptop and have noticed 3 bugs. 2 of them I found in
> > > Bugzilla and replied on them (i915 and Nouveau issues). I am currently
> > > discussing them with an intel engineer.
> > >
> > > On other bug I haven't found so therefore a mail directly to you guys as
> > > maintainers.
> > >
> > > On my other machine, a Samsung NP900X4D (just bought it in the USA, 2017
> > > model), the samsung-laptop.ko module is enabling the use of the keyboard
> > > backlight keys.
> > >
> > > It is not working on my new machine NP900X5N. My samsung-laptop.ko driver
> > > isn't loading. If I try to load it manually it complains about 'no such
> > > device".
> > >
> > > My Linux kernel is working in CSM mode. The module is still not loaded.
> > >
> > That's correct.
> >
> > > As it is weekend I did some more reading and debugging of the module. To
> > > my understanding the module checks the model and type of the laptop. The
> > > known models and types are stored in the struct:
> > >
> > > static struct dmi_system_id __initdata samsung_dmi_table[]
> > >
> > > I wondr if the NP900X5N notebook is included in this list.
> > >
> > > With dmidecode -t chassis it shows:
> > > Getting SMBIOS data from sysfs.
> > > SMBIOS 3.0.0 present.
> > >
> > > Handle 0x0003, DMI type 3, 22 bytes
> > > Chassis Information
> > > Manufacturer: SAMSUNG ELECTRONICS CO., LTD.
> > > Type: Notebook
> > > Lock: Not Present
> > > Version: N/A
> > > Serial Number: 0F4C91CJ900346
> > > Asset Tag: No Asset Tag
> > > Boot-up State: Safe
> > > Power Supply State: Safe
> > > Thermal State: Other
> > > Security Status: None
> > > OEM Information: 0x
> > > Height: Unspecified
> > > Number Of Power Cords: 1
> > > Contained Elements: 0
> > > SKU Number: Chassis
> > >
> > > If I use the -u flag. The notebook value is 0x0A, not 0x10!!!
> > >
> > > Could that be the reason for not loading?
> > Seems likely.
> >
> > > .matches = {
> > > DMI_MATCH(DMI_SYS_VENDOR,
> > > "SAMSUNG ELECTRONICS CO., LTD."),
> > > DMI_MATCH(DMI_CHASSIS_TYPE, "10"), /* Notebook */
> > > },
> > >
> > > Maybe another reason could that that either the i915 and Nouveau modules
> > > are
> > > not working well. I get black screens with the i915 and MMIO faults with
> > > the
> > > nouveau driver. That is another issue that I need to tackle.
> > >
> > I would expect a different error than "no such device" in that case.
> > I think your first thought was correct.
> >
> > As a simple test, I'd suggest replacing "10" with "0A" in the existing
> > DMI_CHASSIS_TYPE match, recompile, and see if it loads and works
> > correctly. Would you be able to test this?
> >
> > > Oh happy new year :-)
> >
> > Happy New Year!
> >
>
>
>
--
Darren Hart
VMware Open Source Technology Center
Re: Please help!: Keyboard backlight not working on my NP900X5N laptop
On Thu, Jan 03, 2019 at 12:20:15AM +0100, Jan Vlietland wrote: > Hi guys, > > Sorry but still walking with this bug under my arm. > > I can't find the right group to file the bug on bugzilla and also on > freenode I cannot find the right group. > > Please help! Hi Jan, I responded to your original email from Dec 31. You will need to allow more time for people to respond as many of us are doing this work on our own time - and this was New Year's Eve/Day. General rule of thumb is to give people a week before following up. We make an effort to respond within 48 business hours, but I find that pretty difficult to keep up with myself. Let's continue the discussion in the original thread. -- Darren Hart VMware Open Source Technology Center
Re: Keyboard backlight not working on my NP900X5N laptop
On Mon, Dec 31, 2018 at 08:40:43PM +0100, Jan Vlietland wrote:
> Hi all,
>
Hey Jan,
> Greg K-H suggested to mail you guys.
>
> I installed Linux 4.20.0-rc7 (downloaded, compiled and installed) on a
> Samsung NP900X5N laptop and have noticed 3 bugs. 2 of them I found in
> Bugzilla and replied on them (i915 and Nouveau issues). I am currently
> discussing them with an intel engineer.
>
> On other bug I haven't found so therefore a mail directly to you guys as
> maintainers.
>
> On my other machine, a Samsung NP900X4D (just bought it in the USA, 2017
> model), the samsung-laptop.ko module is enabling the use of the keyboard
> backlight keys.
>
> It is not working on my new machine NP900X5N. My samsung-laptop.ko driver
> isn't loading. If I try to load it manually it complains about 'no such
> device".
>
> My Linux kernel is working in CSM mode. The module is still not loaded.
>
That's correct.
> As it is weekend I did some more reading and debugging of the module. To my
> understanding the module checks the model and type of the laptop. The known
> models and types are stored in the struct:
>
> static struct dmi_system_id __initdata samsung_dmi_table[]
>
> I wondr if the NP900X5N notebook is included in this list.
>
> With dmidecode -t chassis it shows:
> Getting SMBIOS data from sysfs.
> SMBIOS 3.0.0 present.
>
> Handle 0x0003, DMI type 3, 22 bytes
> Chassis Information
> Manufacturer: SAMSUNG ELECTRONICS CO., LTD.
> Type: Notebook
> Lock: Not Present
> Version: N/A
> Serial Number: 0F4C91CJ900346
> Asset Tag: No Asset Tag
> Boot-up State: Safe
> Power Supply State: Safe
> Thermal State: Other
> Security Status: None
> OEM Information: 0x
> Height: Unspecified
> Number Of Power Cords: 1
> Contained Elements: 0
> SKU Number: Chassis
>
> If I use the -u flag. The notebook value is 0x0A, not 0x10!!!
>
> Could that be the reason for not loading?
Seems likely.
>
> .matches = {
> DMI_MATCH(DMI_SYS_VENDOR,
> "SAMSUNG ELECTRONICS CO., LTD."),
> DMI_MATCH(DMI_CHASSIS_TYPE, "10"), /* Notebook */
> },
>
> Maybe another reason could that that either the i915 and Nouveau modules are
> not working well. I get black screens with the i915 and MMIO faults with the
> nouveau driver. That is another issue that I need to tackle.
>
I would expect a different error than "no such device" in that case.
I think your first thought was correct.
As a simple test, I'd suggest replacing "10" with "0A" in the existing
DMI_CHASSIS_TYPE match, recompile, and see if it loads and works
correctly. Would you be able to test this?
> Oh happy new year :-)
Happy New Year!
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH v2 17/17] power: supply: olpc_battery: Add OLPC XO 1.75 support
On Fri, Nov 16, 2018 at 05:24:03PM +0100, Lubomir Rintel wrote:
> The battery and the protocol are essentially the same as OLPC XO 1.5,
> but the responses from the EC are LSB first.
>
> Signed-off-by: Lubomir Rintel
> Acked-by: Pavel Machek
>
> ---
> Changes since v1:
> - s/s16 ecword_to_cpu/u16 ecword_to_cpu/
> - s/u16 ec_byte/u16 ec_word/
>
> drivers/power/supply/olpc_battery.c | 23 ++-
> 1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/power/supply/olpc_battery.c
> b/drivers/power/supply/olpc_battery.c
...
> @@ -626,6 +635,10 @@ static int olpc_battery_probe(struct platform_device
> *pdev)
> if (ecver > 0x44) {
> /* XO 1 or 1.5 with a new EC firmware. */
> data->new_proto = 1;
> + } else if (of_find_compatible_node(NULL, NULL, "olpc,xo1.75-ec")) {
This if/else blocks concerns me a bit, but I might just be missing some
context.
This tests both ecver as well as the OF compatible string, is this reliable? Do
we know that for all xo1.75-ec compatible nodes the ecver will be <= 0x44? Or,
is ecver undefined? If the latter, then perhaps this test should be performed
first?
if (of_find_compatible_nodex01.75-ec...)
...
else if (ecver > 0x44)
...
else
...
And what happens when ecver == 0x44? We test for > and < but not ==, <=,
or >= in this block
> + /* XO 1.75 */
> + data->new_proto = 1;
> + data->little_endian = 1;
> } else if (ecver < 0x44) {
> /*
>* We've seen a number of EC protocol changes; this driver
> --
> 2.19.1
>
>
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH v2 17/17] power: supply: olpc_battery: Add OLPC XO 1.75 support
On Fri, Nov 16, 2018 at 05:24:03PM +0100, Lubomir Rintel wrote:
> The battery and the protocol are essentially the same as OLPC XO 1.5,
> but the responses from the EC are LSB first.
>
> Signed-off-by: Lubomir Rintel
> Acked-by: Pavel Machek
>
> ---
> Changes since v1:
> - s/s16 ecword_to_cpu/u16 ecword_to_cpu/
> - s/u16 ec_byte/u16 ec_word/
>
> drivers/power/supply/olpc_battery.c | 23 ++-
> 1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/power/supply/olpc_battery.c
> b/drivers/power/supply/olpc_battery.c
...
> @@ -626,6 +635,10 @@ static int olpc_battery_probe(struct platform_device
> *pdev)
> if (ecver > 0x44) {
> /* XO 1 or 1.5 with a new EC firmware. */
> data->new_proto = 1;
> + } else if (of_find_compatible_node(NULL, NULL, "olpc,xo1.75-ec")) {
This if/else blocks concerns me a bit, but I might just be missing some
context.
This tests both ecver as well as the OF compatible string, is this reliable? Do
we know that for all xo1.75-ec compatible nodes the ecver will be <= 0x44? Or,
is ecver undefined? If the latter, then perhaps this test should be performed
first?
if (of_find_compatible_nodex01.75-ec...)
...
else if (ecver > 0x44)
...
else
...
And what happens when ecver == 0x44? We test for > and < but not ==, <=,
or >= in this block
> + /* XO 1.75 */
> + data->new_proto = 1;
> + data->little_endian = 1;
> } else if (ecver < 0x44) {
> /*
>* We've seen a number of EC protocol changes; this driver
> --
> 2.19.1
>
>
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH v2 13/17] power: supply: olpc_battery: Use DT to get battery version
On Fri, Nov 16, 2018 at 05:23:59PM +0100, Lubomir Rintel wrote:
> Avoid using the x86 OLPC platform specific call to get the board
> version. It won't work on FDT-based ARM MMP2 platform.
>
> Signed-off-by: Lubomir Rintel
> Reviewed-by: Andy Shevchenko
> Acked-by: Pavel Machek
>
> ---
> Changes since v1:
> - Sort the new include a bit higher
>
> drivers/power/supply/olpc_battery.c | 10 +++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/power/supply/olpc_battery.c
> b/drivers/power/supply/olpc_battery.c
> index 5a97e42a3547..5323987d9284 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -14,6 +14,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -622,11 +623,13 @@ static int olpc_battery_probe(struct platform_device
> *pdev)
> olpc_ac = power_supply_register(>dev, _ac_desc, NULL);
> if (IS_ERR(olpc_ac))
> return PTR_ERR(olpc_ac);
> -
> - if (olpc_board_at_least(olpc_board_pre(0xd0))) { /* XO-1.5 */
> + if (of_property_match_string(pdev->dev.of_node, "compatible",
> + "olpc,xo1.5-battery") >= 0) {
> + /* XO-1.5 */
> olpc_bat_desc.properties = olpc_xo15_bat_props;
> olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo15_bat_props);
> - } else { /* XO-1 */
> + } else {
> + /* XO-1 */
> olpc_bat_desc.properties = olpc_xo1_bat_props;
> olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo1_bat_props);
> }
> @@ -672,6 +675,7 @@ static int olpc_battery_remove(struct platform_device
> *pdev)
>
> static const struct of_device_id olpc_battery_ids[] = {
> { .compatible = "olpc,xo1-battery" },
> + { .compatible = "olpc,xo1.5-battery" },
The code previously supported xo1.5 (it seems), and the commit message doesn't
mention changing the compatible string. Was this an intentional change? If so,
it should be mentioned in the commit message.
> {}
> };
> MODULE_DEVICE_TABLE(of, olpc_battery_ids);
> --
> 2.19.1
>
>
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH v2 13/17] power: supply: olpc_battery: Use DT to get battery version
On Fri, Nov 16, 2018 at 05:23:59PM +0100, Lubomir Rintel wrote:
> Avoid using the x86 OLPC platform specific call to get the board
> version. It won't work on FDT-based ARM MMP2 platform.
>
> Signed-off-by: Lubomir Rintel
> Reviewed-by: Andy Shevchenko
> Acked-by: Pavel Machek
>
> ---
> Changes since v1:
> - Sort the new include a bit higher
>
> drivers/power/supply/olpc_battery.c | 10 +++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/power/supply/olpc_battery.c
> b/drivers/power/supply/olpc_battery.c
> index 5a97e42a3547..5323987d9284 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -14,6 +14,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -622,11 +623,13 @@ static int olpc_battery_probe(struct platform_device
> *pdev)
> olpc_ac = power_supply_register(>dev, _ac_desc, NULL);
> if (IS_ERR(olpc_ac))
> return PTR_ERR(olpc_ac);
> -
> - if (olpc_board_at_least(olpc_board_pre(0xd0))) { /* XO-1.5 */
> + if (of_property_match_string(pdev->dev.of_node, "compatible",
> + "olpc,xo1.5-battery") >= 0) {
> + /* XO-1.5 */
> olpc_bat_desc.properties = olpc_xo15_bat_props;
> olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo15_bat_props);
> - } else { /* XO-1 */
> + } else {
> + /* XO-1 */
> olpc_bat_desc.properties = olpc_xo1_bat_props;
> olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo1_bat_props);
> }
> @@ -672,6 +675,7 @@ static int olpc_battery_remove(struct platform_device
> *pdev)
>
> static const struct of_device_id olpc_battery_ids[] = {
> { .compatible = "olpc,xo1-battery" },
> + { .compatible = "olpc,xo1.5-battery" },
The code previously supported xo1.5 (it seems), and the commit message doesn't
mention changing the compatible string. Was this an intentional change? If so,
it should be mentioned in the commit message.
> {}
> };
> MODULE_DEVICE_TABLE(of, olpc_battery_ids);
> --
> 2.19.1
>
>
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH v2 07/17] Platform: OLPC: Avoid a warning if the EC didn't register yet
On Fri, Nov 16, 2018 at 05:23:53PM +0100, Lubomir Rintel wrote: > Just return ENODEV, so that whoever attempted to use the EC call can I think you meant EPROBE_DEFER here, but the language in the commit message is a bit ambiguous here... > defer their work. > > Signed-off-by: Lubomir Rintel > Acked-by: Pavel Machek > > --- > Changes since v1: > - EPROBE_DEFER instead of ENODEV > > drivers/platform/olpc/olpc-ec.c | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c > index 35a21c66cd0d..9ee993d5d54b 100644 > --- a/drivers/platform/olpc/olpc-ec.c > +++ b/drivers/platform/olpc/olpc-ec.c > @@ -116,8 +116,11 @@ int olpc_ec_cmd(u8 cmd, u8 *inbuf, size_t inlen, u8 > *outbuf, size_t outlen) > struct olpc_ec_priv *ec = ec_priv; > struct ec_cmd_desc desc; > > - /* Ensure a driver and ec hook have been registered */ > - if (WARN_ON(!ec_driver || !ec_driver->ec_cmd)) > + /* Driver not yet registered. */ > + if (!ec_driver) > + return -EPROBE_DEFER; > + > + if (WARN_ON(!ec_driver->ec_cmd)) > return -ENODEV; > > if (!ec) > -- > 2.19.1 > > -- Darren Hart VMware Open Source Technology Center
Re: [PATCH v2 07/17] Platform: OLPC: Avoid a warning if the EC didn't register yet
On Fri, Nov 16, 2018 at 05:23:53PM +0100, Lubomir Rintel wrote: > Just return ENODEV, so that whoever attempted to use the EC call can I think you meant EPROBE_DEFER here, but the language in the commit message is a bit ambiguous here... > defer their work. > > Signed-off-by: Lubomir Rintel > Acked-by: Pavel Machek > > --- > Changes since v1: > - EPROBE_DEFER instead of ENODEV > > drivers/platform/olpc/olpc-ec.c | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c > index 35a21c66cd0d..9ee993d5d54b 100644 > --- a/drivers/platform/olpc/olpc-ec.c > +++ b/drivers/platform/olpc/olpc-ec.c > @@ -116,8 +116,11 @@ int olpc_ec_cmd(u8 cmd, u8 *inbuf, size_t inlen, u8 > *outbuf, size_t outlen) > struct olpc_ec_priv *ec = ec_priv; > struct ec_cmd_desc desc; > > - /* Ensure a driver and ec hook have been registered */ > - if (WARN_ON(!ec_driver || !ec_driver->ec_cmd)) > + /* Driver not yet registered. */ > + if (!ec_driver) > + return -EPROBE_DEFER; > + > + if (WARN_ON(!ec_driver->ec_cmd)) > return -ENODEV; > > if (!ec) > -- > 2.19.1 > > -- Darren Hart VMware Open Source Technology Center
Re: [PATCH v2 06/17] Platform: OLPC: Add XO-1.75 EC driver
On Fri, Nov 16, 2018 at 05:23:52PM +0100, Lubomir Rintel wrote: > It's based off the driver from the OLPC kernel sources. Somewhat > modernized and cleaned up, for better or worse. > > Modified to plug into the olpc-ec driver infrastructure (so that battery > interface and debugfs could be reused) and the SPI slave framework. > > Signed-off-by: Lubomir Rintel > Hi Lubomir, You asked for some tips on how to incorporate the changes in a patch series like this. Keep in mind that each patch should create an independent small functional change. This makes it easier to review the patch and verify that what you said you were going to do matches what the patch does. For example, if you separate out the style, whitespace, ordering of declarations into an initial first patch, then all that noise is removed when the reviewer goes to check to implementation of one of the features below. This is a large patch, and should most certainly be broken up into several smaller patches. Do cleanups first, followed by functional changes. This ordering ensure that when a "git blame" is used in the future to understand why a given line is what it is, the first hit will be a functional change, and not a cleanup. > --- > Changes since v1: > - Cosmetic style changes; whitespace, ordering of declarations and > #includes, remoted extra comas from sentinels Please make this a separate change, possibly more than one, depending on how many of these there are. This will reduce the size of the subsequent patches, making them easier to review. > - Count the terminating NUL in LOG_BUF_SIZE > - Make olpc_xo175_ec_is_valid_cmd() return -EINVAL instead of -1 > on error > - Spell keyboard/touchpad in full for CHAN_KEYBOARD/TOUCHPAD messages > - Use a #define for PM wakeup processing time > - Log a message on unknown event > - Escape logging payload with %*pE > - Replace an open-coded min() > - Correct an error code on short read > - replaced PM callback #ifdefs with __maybe_unusedm SET_RUNTIME_PM_OPS > and SET_NOIRQ_SYSTEM_SLEEP_PM_OPS > - dev_get_drvdata() instead of a round-trip through platform device > - s/unsigned char x/u8 x/ in olpc_xo175_ec_resume() > - Use GENMASK() instead of 0x for the event mask > - Replace cmd tx/resp rx buffers with structures > - Turned suspend hint arguments into a struct, and tidied up the comment Just from these comments, each of these could be a separate patch. You can group related things together, or those that change the same line or function for example. Order them with cleanups / non-functional-changes first, followed by functional changes. > > Basically all of the above is based on the review by Andy Shevchenko. Andy, what was your intent for Lubomir here? From the above, this looks like it should be several patches to me. Thanks, -- Darren Hart VMware Open Source Technology Center
Re: [PATCH v2 06/17] Platform: OLPC: Add XO-1.75 EC driver
On Fri, Nov 16, 2018 at 05:23:52PM +0100, Lubomir Rintel wrote: > It's based off the driver from the OLPC kernel sources. Somewhat > modernized and cleaned up, for better or worse. > > Modified to plug into the olpc-ec driver infrastructure (so that battery > interface and debugfs could be reused) and the SPI slave framework. > > Signed-off-by: Lubomir Rintel > Hi Lubomir, You asked for some tips on how to incorporate the changes in a patch series like this. Keep in mind that each patch should create an independent small functional change. This makes it easier to review the patch and verify that what you said you were going to do matches what the patch does. For example, if you separate out the style, whitespace, ordering of declarations into an initial first patch, then all that noise is removed when the reviewer goes to check to implementation of one of the features below. This is a large patch, and should most certainly be broken up into several smaller patches. Do cleanups first, followed by functional changes. This ordering ensure that when a "git blame" is used in the future to understand why a given line is what it is, the first hit will be a functional change, and not a cleanup. > --- > Changes since v1: > - Cosmetic style changes; whitespace, ordering of declarations and > #includes, remoted extra comas from sentinels Please make this a separate change, possibly more than one, depending on how many of these there are. This will reduce the size of the subsequent patches, making them easier to review. > - Count the terminating NUL in LOG_BUF_SIZE > - Make olpc_xo175_ec_is_valid_cmd() return -EINVAL instead of -1 > on error > - Spell keyboard/touchpad in full for CHAN_KEYBOARD/TOUCHPAD messages > - Use a #define for PM wakeup processing time > - Log a message on unknown event > - Escape logging payload with %*pE > - Replace an open-coded min() > - Correct an error code on short read > - replaced PM callback #ifdefs with __maybe_unusedm SET_RUNTIME_PM_OPS > and SET_NOIRQ_SYSTEM_SLEEP_PM_OPS > - dev_get_drvdata() instead of a round-trip through platform device > - s/unsigned char x/u8 x/ in olpc_xo175_ec_resume() > - Use GENMASK() instead of 0x for the event mask > - Replace cmd tx/resp rx buffers with structures > - Turned suspend hint arguments into a struct, and tidied up the comment Just from these comments, each of these could be a separate patch. You can group related things together, or those that change the same line or function for example. Order them with cleanups / non-functional-changes first, followed by functional changes. > > Basically all of the above is based on the review by Andy Shevchenko. Andy, what was your intent for Lubomir here? From the above, this looks like it should be several patches to me. Thanks, -- Darren Hart VMware Open Source Technology Center
Re: [PATCH 1/2] platform/x86: wmi: prefix sysfs files in /sys/bus/wmi with the ACPI device
On Sat, Dec 09, 2017 at 01:32:29PM -0800, Andy Lutomirski wrote: > On Fri, Dec 8, 2017 at 7:41 PM, wrote: > >>> On Dec 8, 2017, at 6:34 PM, Mario Limonciello > >>> wrote: > >>> > >>> It's possible for the same GUID to show up on as system twice. > >>> This means using solely the GUID for identify the file will not > >>> be sufficient. > >> > >>Isn't the file already in a per-bus directory? > > > > Yep, but the symlink created in /sys/bus/wmi/devices isn't. > > That's where the kernel complains about duplicate sysfs > > attributes. > > > > It's not exactly a pretty path I submitted, but it does avoid > > those collisions. > > > > Example (with this in place from /sys/bus/wmi/devices): > > lrwxrwxrwx 1 root root 0 Dec 8 21:39 > > PNP0C14:04-70FE8229-D03B-4214-A1C6-1F884B1A892A -> > > ../../../devices/platform/PNP0C14:04/wmi_bus/wmi_bus-PNP0C14:04/PNP0C14:04-70FE8229-D03B-4214-A1C6-1F884B1A892A > > Right, I saw that in the cover letter right after sending this. > > Greg, is there a cleaner way to deal with this? There are two > instances of the same bus type, each of which would like to have a > device called "70FE8229-D03B-4214-A1C6-1F884B1A892A". Can we somehow > rename the symlinks without renaming the device, or are we just > supposed to prefix the device name like Mario is doing here? > Mario, if this is still a concern (I presume it is) would you please resend the patch Cc gregkh directly? -- Darren Hart VMware Open Source Technology Center
Re: [PATCH 1/2] platform/x86: wmi: prefix sysfs files in /sys/bus/wmi with the ACPI device
On Sat, Dec 09, 2017 at 01:32:29PM -0800, Andy Lutomirski wrote: > On Fri, Dec 8, 2017 at 7:41 PM, wrote: > >>> On Dec 8, 2017, at 6:34 PM, Mario Limonciello > >>> wrote: > >>> > >>> It's possible for the same GUID to show up on as system twice. > >>> This means using solely the GUID for identify the file will not > >>> be sufficient. > >> > >>Isn't the file already in a per-bus directory? > > > > Yep, but the symlink created in /sys/bus/wmi/devices isn't. > > That's where the kernel complains about duplicate sysfs > > attributes. > > > > It's not exactly a pretty path I submitted, but it does avoid > > those collisions. > > > > Example (with this in place from /sys/bus/wmi/devices): > > lrwxrwxrwx 1 root root 0 Dec 8 21:39 > > PNP0C14:04-70FE8229-D03B-4214-A1C6-1F884B1A892A -> > > ../../../devices/platform/PNP0C14:04/wmi_bus/wmi_bus-PNP0C14:04/PNP0C14:04-70FE8229-D03B-4214-A1C6-1F884B1A892A > > Right, I saw that in the cover letter right after sending this. > > Greg, is there a cleaner way to deal with this? There are two > instances of the same bus type, each of which would like to have a > device called "70FE8229-D03B-4214-A1C6-1F884B1A892A". Can we somehow > rename the symlinks without renaming the device, or are we just > supposed to prefix the device name like Mario is doing here? > Mario, if this is still a concern (I presume it is) would you please resend the patch Cc gregkh directly? -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] platform/x86: dell-laptop: Mark expected switch fall-throughs
On Thu, Jul 05, 2018 at 03:38:07PM -0500, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
>
> Signed-off-by: Gustavo A. R. Silva
I've applied this patch, but not the acer or sony fall through patches
due to the pending questions there. Happy to revisit those as needed,
but closing these out of the patch queue.
> ---
> drivers/platform/x86/dell-laptop.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/platform/x86/dell-laptop.c
> b/drivers/platform/x86/dell-laptop.c
> index f1fa861..1938f11 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -1565,8 +1565,10 @@ static ssize_t kbd_led_timeout_store(struct device
> *dev,
> switch (unit) {
> case KBD_TIMEOUT_DAYS:
> value *= 24;
> + /* fall through */
> case KBD_TIMEOUT_HOURS:
> value *= 60;
> + /* fall through */
> case KBD_TIMEOUT_MINUTES:
> value *= 60;
> unit = KBD_TIMEOUT_SECONDS;
> --
> 2.7.4
>
>
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] platform/x86: dell-laptop: Mark expected switch fall-throughs
On Thu, Jul 05, 2018 at 03:38:07PM -0500, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
>
> Signed-off-by: Gustavo A. R. Silva
I've applied this patch, but not the acer or sony fall through patches
due to the pending questions there. Happy to revisit those as needed,
but closing these out of the patch queue.
> ---
> drivers/platform/x86/dell-laptop.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/platform/x86/dell-laptop.c
> b/drivers/platform/x86/dell-laptop.c
> index f1fa861..1938f11 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -1565,8 +1565,10 @@ static ssize_t kbd_led_timeout_store(struct device
> *dev,
> switch (unit) {
> case KBD_TIMEOUT_DAYS:
> value *= 24;
> + /* fall through */
> case KBD_TIMEOUT_HOURS:
> value *= 60;
> + /* fall through */
> case KBD_TIMEOUT_MINUTES:
> value *= 60;
> unit = KBD_TIMEOUT_SECONDS;
> --
> 2.7.4
>
>
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] ideapad-laptop: Add Lenovo Yoga 2 13 to the no_hw_rfkill DMI list
On Tue, Nov 13, 2018 at 06:22:45PM -0800, Loic WEI YU NENG wrote:
> Some Lenovo ideapad models lack a physical rfkill switch.
> On Lenovo models Yoga 2 13, ideapad-laptop would wrongly report all
> radios as blocked by hardware which caused wireless network connections
> to fail.
> Add these models without an rfkill switch to the no_hw_rfkill list.
>
> Signed-off-by: Loic WEI YU NENG
Thank you for the patch.
In the future, please be sure to check MAINTAINERS and include the
relevant lists, now Cc'd.
I see this in the context of your patch, but can you confirm the current
driver fails to match your system? It contains the following, which I
would expect to match:
.ident = "Lenovo Yoga 2 11 / 13 / Pro",
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
DMI_MATCH(DMI_BOARD_NAME, "Yoga2"),
},
If it doesn't match, what does dmidecode report for SYS_VENDOR and
BOARD_NAME on your system? Perhaps we need to make the above more
generic.
> ---
> drivers/platform/x86/ideapad-laptop.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/platform/x86/ideapad-laptop.c
> b/drivers/platform/x86/ideapad-laptop.c
> index b6489cba2985..1589dffab9fa 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -1188,6 +1188,13 @@ static const struct dmi_system_id no_hw_rfkill_list[]
> = {
> DMI_MATCH(DMI_BOARD_NAME, "Yoga2"),
> },
> },
> + {
> + .ident = "Lenovo Yoga 2 13",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> + DMI_MATCH(DMI_PRODUCT_VERSION, "Yoga 2 13"),
> + },
> + },
> {
> .ident = "Lenovo Yoga 3 1170 / 1470",
> .matches = {
> --
> 2.17.1
>
>
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] ideapad-laptop: Add Lenovo Yoga 2 13 to the no_hw_rfkill DMI list
On Tue, Nov 13, 2018 at 06:22:45PM -0800, Loic WEI YU NENG wrote:
> Some Lenovo ideapad models lack a physical rfkill switch.
> On Lenovo models Yoga 2 13, ideapad-laptop would wrongly report all
> radios as blocked by hardware which caused wireless network connections
> to fail.
> Add these models without an rfkill switch to the no_hw_rfkill list.
>
> Signed-off-by: Loic WEI YU NENG
Thank you for the patch.
In the future, please be sure to check MAINTAINERS and include the
relevant lists, now Cc'd.
I see this in the context of your patch, but can you confirm the current
driver fails to match your system? It contains the following, which I
would expect to match:
.ident = "Lenovo Yoga 2 11 / 13 / Pro",
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
DMI_MATCH(DMI_BOARD_NAME, "Yoga2"),
},
If it doesn't match, what does dmidecode report for SYS_VENDOR and
BOARD_NAME on your system? Perhaps we need to make the above more
generic.
> ---
> drivers/platform/x86/ideapad-laptop.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/platform/x86/ideapad-laptop.c
> b/drivers/platform/x86/ideapad-laptop.c
> index b6489cba2985..1589dffab9fa 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -1188,6 +1188,13 @@ static const struct dmi_system_id no_hw_rfkill_list[]
> = {
> DMI_MATCH(DMI_BOARD_NAME, "Yoga2"),
> },
> },
> + {
> + .ident = "Lenovo Yoga 2 13",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> + DMI_MATCH(DMI_PRODUCT_VERSION, "Yoga 2 13"),
> + },
> + },
> {
> .ident = "Lenovo Yoga 3 1170 / 1470",
> .matches = {
> --
> 2.17.1
>
>
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] scripts/kconfig/merge_config: don't redefine 'y' to 'm'
On Fri, Nov 02, 2018 at 12:41:19PM +0100, Anders Roxell wrote:
> In today's merge_config.sh the order of the config fragment files dictates
> the output of a config option. With this approach we will get different
> .config files depending on the order of the config fragment files.
> Adding a switch to add precedence for builtin over modules, this will
> make the .config file the same
>
> Suggested-by: Arnd Bergmann
> Signed-off-by: Anders Roxell
Thanks for the patch Anders!
> ---
> scripts/kconfig/merge_config.sh | 15 ++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
> index da66e7742282..902a60b45614 100755
> --- a/scripts/kconfig/merge_config.sh
> +++ b/scripts/kconfig/merge_config.sh
> @@ -32,6 +32,7 @@ usage() {
> echo " -monly merge the fragments, do not execute the make command"
> echo " -nuse allnoconfig instead of alldefconfig"
> echo " -rlist redundant entries when merging fragments"
> + echo " -ymake builtin have precedence over modules"
> echo " -Odir to put generated output files. Consider setting
> \$KCONFIG_CONFIG instead."
> echo
> echo "Used prefix: '$CONFIG_PREFIX'. You can redefine it with \$CONFIG_
> environment variable."
> @@ -40,6 +41,7 @@ usage() {
> RUNMAKE=true
> ALLTARGET=alldefconfig
> WARNREDUN=false
> +BUILTIN=false
> OUTPUT=.
> CONFIG_PREFIX=${CONFIG_-CONFIG_}
>
> @@ -64,6 +66,11 @@ while true; do
> shift
> continue
> ;;
> + "-y")
> + BUILTIN=true
> + shift
> + continue
> + ;;
> "-O")
> if [ -d $2 ];then
> OUTPUT=$(echo $2 | sed 's/\/*$//')
> @@ -122,7 +129,13 @@ for MERGE_FILE in $MERGE_LIST ; do
> grep -q -w $CFG $TMP_FILE || continue
> PREV_VAL=$(grep -w $CFG $TMP_FILE)
> NEW_VAL=$(grep -w $CFG $MERGE_FILE)
> - if [ "x$PREV_VAL" != "x$NEW_VAL" ] ; then
> + if test "$BUILTIN" = "true" && echo $PREV_VAL |grep -Eq
> '^\w+=y' && echo $NEW_VAL |grep -Eq '^\w+=m' ; then
I think we can avoid the rather lengthy/forky "echo | grep" mechanism
with POSIX substring parameter expansion (should work in bash, dash,
etc.).
http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_06_02
We can also be more consistent with the other tests in the script, consider:
if [ "$BUILTIN" = "true" ] && [ "${NEW_VAL#CONFIG_*=}" = "m" ]
&& [ "${PREV_VAL#CONFIG_*=}" = "y" ]; then
> + echo Value of $CFG is \'y\' and we don\'t want to
> redefine the fragment $MERGE_FILE:
I think we can drop the above and perhaps update the last line with...
> + echo Previous value: $PREV_VAL
> + echo New value: $NEW_VAL
> + echo Will use previous value.
echo "-y passed, will not demote y to m"
Or something along those lines.
> + echo
> + elif [ "x$PREV_VAL" != "x$NEW_VAL" ] ; then
> echo Value of $CFG is redefined by fragment $MERGE_FILE:
> echo Previous value: $PREV_VAL
> echo New value: $NEW_VAL
> --
> 2.11.0
>
>
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] scripts/kconfig/merge_config: don't redefine 'y' to 'm'
On Fri, Nov 02, 2018 at 12:41:19PM +0100, Anders Roxell wrote:
> In today's merge_config.sh the order of the config fragment files dictates
> the output of a config option. With this approach we will get different
> .config files depending on the order of the config fragment files.
> Adding a switch to add precedence for builtin over modules, this will
> make the .config file the same
>
> Suggested-by: Arnd Bergmann
> Signed-off-by: Anders Roxell
Thanks for the patch Anders!
> ---
> scripts/kconfig/merge_config.sh | 15 ++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
> index da66e7742282..902a60b45614 100755
> --- a/scripts/kconfig/merge_config.sh
> +++ b/scripts/kconfig/merge_config.sh
> @@ -32,6 +32,7 @@ usage() {
> echo " -monly merge the fragments, do not execute the make command"
> echo " -nuse allnoconfig instead of alldefconfig"
> echo " -rlist redundant entries when merging fragments"
> + echo " -ymake builtin have precedence over modules"
> echo " -Odir to put generated output files. Consider setting
> \$KCONFIG_CONFIG instead."
> echo
> echo "Used prefix: '$CONFIG_PREFIX'. You can redefine it with \$CONFIG_
> environment variable."
> @@ -40,6 +41,7 @@ usage() {
> RUNMAKE=true
> ALLTARGET=alldefconfig
> WARNREDUN=false
> +BUILTIN=false
> OUTPUT=.
> CONFIG_PREFIX=${CONFIG_-CONFIG_}
>
> @@ -64,6 +66,11 @@ while true; do
> shift
> continue
> ;;
> + "-y")
> + BUILTIN=true
> + shift
> + continue
> + ;;
> "-O")
> if [ -d $2 ];then
> OUTPUT=$(echo $2 | sed 's/\/*$//')
> @@ -122,7 +129,13 @@ for MERGE_FILE in $MERGE_LIST ; do
> grep -q -w $CFG $TMP_FILE || continue
> PREV_VAL=$(grep -w $CFG $TMP_FILE)
> NEW_VAL=$(grep -w $CFG $MERGE_FILE)
> - if [ "x$PREV_VAL" != "x$NEW_VAL" ] ; then
> + if test "$BUILTIN" = "true" && echo $PREV_VAL |grep -Eq
> '^\w+=y' && echo $NEW_VAL |grep -Eq '^\w+=m' ; then
I think we can avoid the rather lengthy/forky "echo | grep" mechanism
with POSIX substring parameter expansion (should work in bash, dash,
etc.).
http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_06_02
We can also be more consistent with the other tests in the script, consider:
if [ "$BUILTIN" = "true" ] && [ "${NEW_VAL#CONFIG_*=}" = "m" ]
&& [ "${PREV_VAL#CONFIG_*=}" = "y" ]; then
> + echo Value of $CFG is \'y\' and we don\'t want to
> redefine the fragment $MERGE_FILE:
I think we can drop the above and perhaps update the last line with...
> + echo Previous value: $PREV_VAL
> + echo New value: $NEW_VAL
> + echo Will use previous value.
echo "-y passed, will not demote y to m"
Or something along those lines.
> + echo
> + elif [ "x$PREV_VAL" != "x$NEW_VAL" ] ; then
> echo Value of $CFG is redefined by fragment $MERGE_FILE:
> echo Previous value: $PREV_VAL
> echo New value: $NEW_VAL
> --
> 2.11.0
>
>
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] scripts/kconfig/merge_config: don't redefine 'y' to 'm'
On Tue, Nov 06, 2018 at 02:57:40PM +0100, Anders Roxell wrote: > On Mon, 5 Nov 2018 at 09:35, Masahiro Yamada > wrote: > > > > Hi Anders, > > > > On Fri, Nov 2, 2018 at 8:41 PM Anders Roxell > > wrote: > > > > > > In today's merge_config.sh the order of the config fragment files dictates > > > the output of a config option. With this approach we will get different > > > .config files depending on the order of the config fragment files. > > > Adding a switch to add precedence for builtin over modules, this will > > > make the .config file the same > > > > > > Suggested-by: Arnd Bergmann > > > Signed-off-by: Anders Roxell > > > --- > > > > I think this patch makes sense. > > > > Just in case, could you please provide me the context of the discussion? > > For instance we don't want to force X86 from DRM=y to DRM=m, when > enabling selftest, that would surely break somebody's setup and you also > don't want to force ARM64 from DRM=m to DRM=y, that seems > unnecessary for a big subsystem like DRM. > > > > > Does the real problem exist in the kernel tree, > > Not that I'm aware about. > > Cheers, > Anders > > > or for local fragment files? This is really about ordering of fragment files and not "making a symbol 'less'", I think were Arnd's words. So, doing something like: $ make arm64-selftest.config drm.config where arm64-selftest.config defines DRM=y and drm.config defines DRM=m, the result should be "DRM=y". So the first step is to make merge_config.sh support it. A follow up step would be to integrate this into the kernel Makefile system if we determine this is the correct behavior, or to provide a parameter if we don't want to change the default behavior. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] scripts/kconfig/merge_config: don't redefine 'y' to 'm'
On Tue, Nov 06, 2018 at 02:57:40PM +0100, Anders Roxell wrote: > On Mon, 5 Nov 2018 at 09:35, Masahiro Yamada > wrote: > > > > Hi Anders, > > > > On Fri, Nov 2, 2018 at 8:41 PM Anders Roxell > > wrote: > > > > > > In today's merge_config.sh the order of the config fragment files dictates > > > the output of a config option. With this approach we will get different > > > .config files depending on the order of the config fragment files. > > > Adding a switch to add precedence for builtin over modules, this will > > > make the .config file the same > > > > > > Suggested-by: Arnd Bergmann > > > Signed-off-by: Anders Roxell > > > --- > > > > I think this patch makes sense. > > > > Just in case, could you please provide me the context of the discussion? > > For instance we don't want to force X86 from DRM=y to DRM=m, when > enabling selftest, that would surely break somebody's setup and you also > don't want to force ARM64 from DRM=m to DRM=y, that seems > unnecessary for a big subsystem like DRM. > > > > > Does the real problem exist in the kernel tree, > > Not that I'm aware about. > > Cheers, > Anders > > > or for local fragment files? This is really about ordering of fragment files and not "making a symbol 'less'", I think were Arnd's words. So, doing something like: $ make arm64-selftest.config drm.config where arm64-selftest.config defines DRM=y and drm.config defines DRM=m, the result should be "DRM=y". So the first step is to make merge_config.sh support it. A follow up step would be to integrate this into the kernel Makefile system if we determine this is the correct behavior, or to provide a parameter if we don't want to change the default behavior. -- Darren Hart VMware Open Source Technology Center
Re: Linux 4.20-rc1
On Sun, Nov 04, 2018 at 04:12:45PM -0800, Linus Torvalds wrote: > > Because yes, the merge window is two weeks, but it's two weeks partly > exactly _because_ people (not just me) sometimes need extra time to > resolve any possible issues, not because regular everyday pull > requests should spread out over the whole two weeks. The development > for things meant for the next release should have been done by the > time the merge window opens. > > Anyway, let's see. Maybe it won't be needed. It hasn't become a > problem, it just was starting to feel a bit tight there. I'm curious what others find to be the cause of hitting the second half of the merge window. For my part, I have traditionally monitored the tail end of the RC period waiting for the version tag to land. Other times, like this time, work and travel get hectic, and I realize later than I should that it was time to send in the PR. I imagine many of us have just written notification scripts for version tags, or emails from you matching certain subject patterns - but for my part, a more predictable end of the RC period and/or an explicit email to maintainers listed in the MAINTAINER file would be helpful. Would you consider adding a step to your git tag process to scan the MAINTAINERS file and send us a "Ready your PRs!" email with a note about preferring to get these during the first week wherever possible? This seems like it could be easily automated, easily filtered by those that might not want it, and reduces the number of times you have to explain your preference as new maintainers come and go. -- Darren Hart VMware Open Source Technology Center
Re: Linux 4.20-rc1
On Sun, Nov 04, 2018 at 04:12:45PM -0800, Linus Torvalds wrote: > > Because yes, the merge window is two weeks, but it's two weeks partly > exactly _because_ people (not just me) sometimes need extra time to > resolve any possible issues, not because regular everyday pull > requests should spread out over the whole two weeks. The development > for things meant for the next release should have been done by the > time the merge window opens. > > Anyway, let's see. Maybe it won't be needed. It hasn't become a > problem, it just was starting to feel a bit tight there. I'm curious what others find to be the cause of hitting the second half of the merge window. For my part, I have traditionally monitored the tail end of the RC period waiting for the version tag to land. Other times, like this time, work and travel get hectic, and I realize later than I should that it was time to send in the PR. I imagine many of us have just written notification scripts for version tags, or emails from you matching certain subject patterns - but for my part, a more predictable end of the RC period and/or an explicit email to maintainers listed in the MAINTAINER file would be helpful. Would you consider adding a step to your git tag process to scan the MAINTAINERS file and send us a "Ready your PRs!" email with a note about preferring to get these during the first week wherever possible? This seems like it could be easily automated, easily filtered by those that might not want it, and reduces the number of times you have to explain your preference as new maintainers come and go. -- Darren Hart VMware Open Source Technology Center
[GIT PULL] platform-drivers-x86 for 4.20-1
Hi Linus, This tag has a few merge conflicts with some x86/cpu id changes. Stephen Rothwell's merge in next/master is correct, and I have provided an example here as well: git://git.infradead.org/linux-platform-drivers-x86.git pdx86-v4.20-1-merge Thanks, Darren Hart VMware Open Source Technology Center The following changes since commit ff0e9f26288d2daee4950f42b37a3d3d30d36ec1: platform/x86: alienware-wmi: Correct a memory leak (2018-09-10 13:45:43 -0700) are available in the git repository at: git://git.infradead.org/linux-platform-drivers-x86.git tags/platform-drivers-x86-v4.20-1 for you to fetch changes up to 3b692c55e58d06ba9b17c66784cab5a95ba5be9b: HID: asus: only support backlight when it's not driven by WMI (2018-10-31 16:11:40 +0200) platform-drivers-x86 for v4.20-1 Move the Dell dcdbas and dell_rbu drivers into platform/drivers/x86 as they are closely coupled with other drivers in this location. Improve _init* usage for acerhdf and fix some usage issues with messages and module parameters. Simplify asus-wmi by calling ACPI/WMI methods directly, eliminating workqueue overhead, eliminate double reporting of keyboard backlight. Fix wake from USB failure on Bay Trail devices (intel_int0002_vgpio). Notify intel_telemetry users when IPC1 device is not enabled. Update various drivers with new laptop model IDs. Update several intel drivers to use SPDX identifers and order headers alphabetically. The following is an automated git shortlog grouped by driver: Add Intel AtomISP2 dummy / power-management driver: - Add Intel AtomISP2 dummy / power-management driver lg-laptop: - Add LG Gram laptop special features driver HID: - asus: only support backlight when it's not driven by WMI MAINTAINERS: - intel_telemetry: Update maintainers info - intel_pmc_core: Update MAINTAINERS - Update maintainer for dcdbas and dell_rbu - Use my infradead account exclusively for PDx86 work acerhdf: - restructure to allow large BIOS table be __initconst - mark appropriate content with __init prefix - Add BIOS entry for Gateway LT31 v1.3307 - Remove cut-and-paste trap from instructions - Enable ability to list supported systems - clarify modinfo messages for BIOS override asus-wmi: - export function for evaluating WMI methods - Only notify kbd LED hw_change by fn-key pressed - Simplify the keyboard brightness updating process firmware: - dcdbas: include linux/io.h - dcdbas: Move dcdbas to drivers/platform/x86 - dell_rbu: Move dell_rbu to drivers/platform/x86 - dcdbas: Add support for WSMT ACPI table - dell_rbu: Make payload memory uncachable ideapad-laptop: - Add Y530-15ICH to no_hw_rfkill - Use __func__ instead of read_ec_cmd in pr_err intel-hid: - Convert to use SPDX identifier intel-ips: - Convert to use SPDX identifier intel-rst: - Convert to use SPDX identifier - Sort headers alphabetically intel-smartconnect: - Convert to use SPDX identifier - Sort headers alphabetically intel-wmi-thunderbolt: - Add dynamic debugging - Convert to use SPDX identifier intel_bxtwc_tmu: - Convert to use SPDX identifier intel_cht_int33fe: - Convert to use SPDX identifier intel_chtdc_ti_pwrbtn: - Add SPDX identifier intel_int0002_vgpio: - Convert to use SPDX identifier - Implement irq_set_wake - Enable the driver on Bay Trail platforms intel_menlow: - Convert to use SPDX identifier - Sort headers alphabetically intel_mid_powerbtn: - Convert to use SPDX identifier - Remove unnecessary init.h inclusion - Get rid of custom ICPU() macro intel_mid_thermal: - Convert to use SPDX identifier - Sort headers alphabetically intel_oaktrail: - Convert to use SPDX identifier - Sort headers alphabetically intel_pmc: - Convert to use SPDX identifier - Sort headers alphabetically intel_punit_ipc: - Convert to use SPDX identifier - Sort headers alphabetically intel_scu_ipc: - Convert to use SPDX identifier - Sort headers alphabetically intel_telemetry: - Get rid of custom macro - report debugfs failure - Convert to use SPDX identifier intel_turbo_max_3: - Convert to use SPDX identifier - Sort headers alphabetically mlx-platform: - Properly use mlxplat_mlxcpld_msn201x_items touchscreen_dmi: - Add min-x and min-y settings for various models - Add info for the Onda V80 Plus v3 tablet - Add info for the Trekstor Primetab T13B tablet - Add info for the Trekstor Primebook C11 convertible tracing: - Trivia spelling fix containerof() -> container_of() wmi: - declare device_type structure as constant Andy Shevchenko (32): tracing: Trivia spelling fix containerof() -> container_of() MAINTAINERS: Use my infradead account exclusively for PDx86 work platform/x86: intel_mid_powerbtn: Get rid of custom ICPU() macro platform/x86: intel_bxt
[GIT PULL] platform-drivers-x86 for 4.20-1
Hi Linus, This tag has a few merge conflicts with some x86/cpu id changes. Stephen Rothwell's merge in next/master is correct, and I have provided an example here as well: git://git.infradead.org/linux-platform-drivers-x86.git pdx86-v4.20-1-merge Thanks, Darren Hart VMware Open Source Technology Center The following changes since commit ff0e9f26288d2daee4950f42b37a3d3d30d36ec1: platform/x86: alienware-wmi: Correct a memory leak (2018-09-10 13:45:43 -0700) are available in the git repository at: git://git.infradead.org/linux-platform-drivers-x86.git tags/platform-drivers-x86-v4.20-1 for you to fetch changes up to 3b692c55e58d06ba9b17c66784cab5a95ba5be9b: HID: asus: only support backlight when it's not driven by WMI (2018-10-31 16:11:40 +0200) platform-drivers-x86 for v4.20-1 Move the Dell dcdbas and dell_rbu drivers into platform/drivers/x86 as they are closely coupled with other drivers in this location. Improve _init* usage for acerhdf and fix some usage issues with messages and module parameters. Simplify asus-wmi by calling ACPI/WMI methods directly, eliminating workqueue overhead, eliminate double reporting of keyboard backlight. Fix wake from USB failure on Bay Trail devices (intel_int0002_vgpio). Notify intel_telemetry users when IPC1 device is not enabled. Update various drivers with new laptop model IDs. Update several intel drivers to use SPDX identifers and order headers alphabetically. The following is an automated git shortlog grouped by driver: Add Intel AtomISP2 dummy / power-management driver: - Add Intel AtomISP2 dummy / power-management driver lg-laptop: - Add LG Gram laptop special features driver HID: - asus: only support backlight when it's not driven by WMI MAINTAINERS: - intel_telemetry: Update maintainers info - intel_pmc_core: Update MAINTAINERS - Update maintainer for dcdbas and dell_rbu - Use my infradead account exclusively for PDx86 work acerhdf: - restructure to allow large BIOS table be __initconst - mark appropriate content with __init prefix - Add BIOS entry for Gateway LT31 v1.3307 - Remove cut-and-paste trap from instructions - Enable ability to list supported systems - clarify modinfo messages for BIOS override asus-wmi: - export function for evaluating WMI methods - Only notify kbd LED hw_change by fn-key pressed - Simplify the keyboard brightness updating process firmware: - dcdbas: include linux/io.h - dcdbas: Move dcdbas to drivers/platform/x86 - dell_rbu: Move dell_rbu to drivers/platform/x86 - dcdbas: Add support for WSMT ACPI table - dell_rbu: Make payload memory uncachable ideapad-laptop: - Add Y530-15ICH to no_hw_rfkill - Use __func__ instead of read_ec_cmd in pr_err intel-hid: - Convert to use SPDX identifier intel-ips: - Convert to use SPDX identifier intel-rst: - Convert to use SPDX identifier - Sort headers alphabetically intel-smartconnect: - Convert to use SPDX identifier - Sort headers alphabetically intel-wmi-thunderbolt: - Add dynamic debugging - Convert to use SPDX identifier intel_bxtwc_tmu: - Convert to use SPDX identifier intel_cht_int33fe: - Convert to use SPDX identifier intel_chtdc_ti_pwrbtn: - Add SPDX identifier intel_int0002_vgpio: - Convert to use SPDX identifier - Implement irq_set_wake - Enable the driver on Bay Trail platforms intel_menlow: - Convert to use SPDX identifier - Sort headers alphabetically intel_mid_powerbtn: - Convert to use SPDX identifier - Remove unnecessary init.h inclusion - Get rid of custom ICPU() macro intel_mid_thermal: - Convert to use SPDX identifier - Sort headers alphabetically intel_oaktrail: - Convert to use SPDX identifier - Sort headers alphabetically intel_pmc: - Convert to use SPDX identifier - Sort headers alphabetically intel_punit_ipc: - Convert to use SPDX identifier - Sort headers alphabetically intel_scu_ipc: - Convert to use SPDX identifier - Sort headers alphabetically intel_telemetry: - Get rid of custom macro - report debugfs failure - Convert to use SPDX identifier intel_turbo_max_3: - Convert to use SPDX identifier - Sort headers alphabetically mlx-platform: - Properly use mlxplat_mlxcpld_msn201x_items touchscreen_dmi: - Add min-x and min-y settings for various models - Add info for the Onda V80 Plus v3 tablet - Add info for the Trekstor Primetab T13B tablet - Add info for the Trekstor Primebook C11 convertible tracing: - Trivia spelling fix containerof() -> container_of() wmi: - declare device_type structure as constant Andy Shevchenko (32): tracing: Trivia spelling fix containerof() -> container_of() MAINTAINERS: Use my infradead account exclusively for PDx86 work platform/x86: intel_mid_powerbtn: Get rid of custom ICPU() macro platform/x86: intel_bxt
[GIT PULL] platform-drivers-x86 for 4.19-2
Hi Linus and Greg, The following changes since commit 5b394b2ddf0347bef56e50c69a58773c94343ff3: Linux 4.19-rc1 (2018-08-26 14:11:59 -0700) are available in the Git repository at: git://git.infradead.org/linux-platform-drivers-x86.git tags/platform-drivers-x86-v4.19-2 for you to fetch changes up to ff0e9f26288d2daee4950f42b37a3d3d30d36ec1: platform/x86: alienware-wmi: Correct a memory leak (2018-09-10 13:45:43 -0700) Thanks, Darren Hart VMware Open Source Technology Center platform-drivers-x86 for v4.19-2 Free allocated ACPI buffers in two drivers. The following is an automated git shortlog grouped by driver: alienware-wmi: - Correct a memory leak dell-smbios-wmi: - Correct a memory leak Mario Limonciello (2): platform/x86: dell-smbios-wmi: Correct a memory leak platform/x86: alienware-wmi: Correct a memory leak drivers/platform/x86/alienware-wmi.c | 1 + drivers/platform/x86/dell-smbios-wmi.c | 1 + 2 files changed, 2 insertions(+) -- Darren Hart VMware Open Source Technology Center
[GIT PULL] platform-drivers-x86 for 4.19-2
Hi Linus and Greg, The following changes since commit 5b394b2ddf0347bef56e50c69a58773c94343ff3: Linux 4.19-rc1 (2018-08-26 14:11:59 -0700) are available in the Git repository at: git://git.infradead.org/linux-platform-drivers-x86.git tags/platform-drivers-x86-v4.19-2 for you to fetch changes up to ff0e9f26288d2daee4950f42b37a3d3d30d36ec1: platform/x86: alienware-wmi: Correct a memory leak (2018-09-10 13:45:43 -0700) Thanks, Darren Hart VMware Open Source Technology Center platform-drivers-x86 for v4.19-2 Free allocated ACPI buffers in two drivers. The following is an automated git shortlog grouped by driver: alienware-wmi: - Correct a memory leak dell-smbios-wmi: - Correct a memory leak Mario Limonciello (2): platform/x86: dell-smbios-wmi: Correct a memory leak platform/x86: alienware-wmi: Correct a memory leak drivers/platform/x86/alienware-wmi.c | 1 + drivers/platform/x86/dell-smbios-wmi.c | 1 + 2 files changed, 2 insertions(+) -- Darren Hart VMware Open Source Technology Center
Re: [PATCH 1/2] platform/x86: dell-smbios-wmi: Correct a memory leak
On Wed, Sep 12, 2018 at 12:14:34PM +, mario.limoncie...@dell.com wrote: > Darren, > > > -Original Message- > > From: Darren Hart [mailto:dvh...@infradead.org] > > Sent: Monday, September 10, 2018 3:52 PM > > To: Limonciello, Mario > > Cc: Andy Shevchenko; LKML; platform-driver-...@vger.kernel.org; > > sta...@vger.kernel.org > > Subject: Re: [PATCH 1/2] platform/x86: dell-smbios-wmi: Correct a memory > > leak > > > > On Mon, Sep 10, 2018 at 01:01:52PM -0500, Mario Limonciello wrote: > > > ACPI buffers were being allocated but never freed. > > > > > > Reported-by: Pinzhen Xu > > > Signed-off-by: Mario Limonciello > > > Cc: sta...@vger.kernel.org > > > > Thanks Mario and Pinzhen, both queued. That's an easy one to miss, the > > usage not > > being particularly obvious. > > > > This cropped up and is fixed by that patch: > https://github.com/dell/libsmbios/issues/63 > > Given the impact and how small the patch is, I think it might be good if this > can come to 4.19-rcX if possible. > Thoughts? > > I suspect this one might be the same issue too: > https://bugzilla.redhat.com/show_bug.cgi?id=1627609 Happy to pull these into fixes for 4.19-rcX. Queued to Fixes. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH 1/2] platform/x86: dell-smbios-wmi: Correct a memory leak
On Wed, Sep 12, 2018 at 12:14:34PM +, mario.limoncie...@dell.com wrote: > Darren, > > > -Original Message- > > From: Darren Hart [mailto:dvh...@infradead.org] > > Sent: Monday, September 10, 2018 3:52 PM > > To: Limonciello, Mario > > Cc: Andy Shevchenko; LKML; platform-driver-...@vger.kernel.org; > > sta...@vger.kernel.org > > Subject: Re: [PATCH 1/2] platform/x86: dell-smbios-wmi: Correct a memory > > leak > > > > On Mon, Sep 10, 2018 at 01:01:52PM -0500, Mario Limonciello wrote: > > > ACPI buffers were being allocated but never freed. > > > > > > Reported-by: Pinzhen Xu > > > Signed-off-by: Mario Limonciello > > > Cc: sta...@vger.kernel.org > > > > Thanks Mario and Pinzhen, both queued. That's an easy one to miss, the > > usage not > > being particularly obvious. > > > > This cropped up and is fixed by that patch: > https://github.com/dell/libsmbios/issues/63 > > Given the impact and how small the patch is, I think it might be good if this > can come to 4.19-rcX if possible. > Thoughts? > > I suspect this one might be the same issue too: > https://bugzilla.redhat.com/show_bug.cgi?id=1627609 Happy to pull these into fixes for 4.19-rcX. Queued to Fixes. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] platform/x86: sony-laptop: Mark expected switch fall-through
On Sun, Jul 08, 2018 at 03:56:17AM +0900, Mattia Dongili wrote:
> On Fri, Jul 06, 2018 at 04:24:27PM -0700, Darren Hart wrote:
> > On Thu, Jul 05, 2018 at 03:47:03PM -0500, Gustavo A. R. Silva wrote:
> > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> > > where we are expecting to fall through.
> > >
> > > Signed-off-by: Gustavo A. R. Silva
> > > ---
> > > drivers/platform/x86/sony-laptop.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/platform/x86/sony-laptop.c
> > > b/drivers/platform/x86/sony-laptop.c
> > > index b205b03..e614cb7 100644
> > > --- a/drivers/platform/x86/sony-laptop.c
> > > +++ b/drivers/platform/x86/sony-laptop.c
> > > @@ -4427,6 +4427,7 @@ sony_pic_read_possible_resource(struct
> > > acpi_resource *resource, void *context)
> > > default:
> > > dprintk("Resource %d isn't an IRQ nor an IO port\n",
> > > resource->type);
> > > + /* fall through */
> >
> > Here too, I wonder if this is intentional. Either way, from what I can see,
> > the
> > final line in the function:
> >
> > return AE_CTRL_TERMINATE;
> >
> > Is unreachable as there are no "break" statements in the switch, and the
> > default
> > falls through to return AE_OK. Something doesn't seem right here.
>
> I think so too. Looks to me that the default case was meant to return
> AE_CTRL_TERMINATE (i.e. swapping it to be last would do).
>
> Having written that code aeons ago, I'm not sure if there is a good
> reason to be like that, though it still looks like a bug (that
> existed ever since...).
>
> Want to fix it as part of this patch series? Or I can send a patch
> myself, let me know.
Mattia,
I'd prefer to decouple the fix from the implicit fall through discussion. Please
send a separate fix.
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] platform/x86: sony-laptop: Mark expected switch fall-through
On Sun, Jul 08, 2018 at 03:56:17AM +0900, Mattia Dongili wrote:
> On Fri, Jul 06, 2018 at 04:24:27PM -0700, Darren Hart wrote:
> > On Thu, Jul 05, 2018 at 03:47:03PM -0500, Gustavo A. R. Silva wrote:
> > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> > > where we are expecting to fall through.
> > >
> > > Signed-off-by: Gustavo A. R. Silva
> > > ---
> > > drivers/platform/x86/sony-laptop.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/platform/x86/sony-laptop.c
> > > b/drivers/platform/x86/sony-laptop.c
> > > index b205b03..e614cb7 100644
> > > --- a/drivers/platform/x86/sony-laptop.c
> > > +++ b/drivers/platform/x86/sony-laptop.c
> > > @@ -4427,6 +4427,7 @@ sony_pic_read_possible_resource(struct
> > > acpi_resource *resource, void *context)
> > > default:
> > > dprintk("Resource %d isn't an IRQ nor an IO port\n",
> > > resource->type);
> > > + /* fall through */
> >
> > Here too, I wonder if this is intentional. Either way, from what I can see,
> > the
> > final line in the function:
> >
> > return AE_CTRL_TERMINATE;
> >
> > Is unreachable as there are no "break" statements in the switch, and the
> > default
> > falls through to return AE_OK. Something doesn't seem right here.
>
> I think so too. Looks to me that the default case was meant to return
> AE_CTRL_TERMINATE (i.e. swapping it to be last would do).
>
> Having written that code aeons ago, I'm not sure if there is a good
> reason to be like that, though it still looks like a bug (that
> existed ever since...).
>
> Want to fix it as part of this patch series? Or I can send a patch
> myself, let me know.
Mattia,
I'd prefer to decouple the fix from the implicit fall through discussion. Please
send a separate fix.
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH 1/2] platform/x86: dell-smbios-wmi: Correct a memory leak
On Mon, Sep 10, 2018 at 01:01:52PM -0500, Mario Limonciello wrote: > ACPI buffers were being allocated but never freed. > > Reported-by: Pinzhen Xu > Signed-off-by: Mario Limonciello > Cc: sta...@vger.kernel.org Thanks Mario and Pinzhen, both queued. That's an easy one to miss, the usage not being particularly obvious. > --- > drivers/platform/x86/dell-smbios-wmi.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/platform/x86/dell-smbios-wmi.c > b/drivers/platform/x86/dell-smbios-wmi.c > index 88afe56..cf2229e 100644 > --- a/drivers/platform/x86/dell-smbios-wmi.c > +++ b/drivers/platform/x86/dell-smbios-wmi.c > @@ -78,6 +78,7 @@ static int run_smbios_call(struct wmi_device *wdev) > dev_dbg(>dev, "result: [%08x,%08x,%08x,%08x]\n", > priv->buf->std.output[0], priv->buf->std.output[1], > priv->buf->std.output[2], priv->buf->std.output[3]); > + kfree(output.pointer); > > return 0; > } > -- > 2.7.4 > > -- Darren Hart VMware Open Source Technology Center
Re: [PATCH 1/2] platform/x86: dell-smbios-wmi: Correct a memory leak
On Mon, Sep 10, 2018 at 01:01:52PM -0500, Mario Limonciello wrote: > ACPI buffers were being allocated but never freed. > > Reported-by: Pinzhen Xu > Signed-off-by: Mario Limonciello > Cc: sta...@vger.kernel.org Thanks Mario and Pinzhen, both queued. That's an easy one to miss, the usage not being particularly obvious. > --- > drivers/platform/x86/dell-smbios-wmi.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/platform/x86/dell-smbios-wmi.c > b/drivers/platform/x86/dell-smbios-wmi.c > index 88afe56..cf2229e 100644 > --- a/drivers/platform/x86/dell-smbios-wmi.c > +++ b/drivers/platform/x86/dell-smbios-wmi.c > @@ -78,6 +78,7 @@ static int run_smbios_call(struct wmi_device *wdev) > dev_dbg(>dev, "result: [%08x,%08x,%08x,%08x]\n", > priv->buf->std.output[0], priv->buf->std.output[1], > priv->buf->std.output[2], priv->buf->std.output[3]); > + kfree(output.pointer); > > return 0; > } > -- > 2.7.4 > > -- Darren Hart VMware Open Source Technology Center
Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods
On Mon, Jul 09, 2018 at 09:08:56PM +, mario.limoncie...@dell.com wrote: > > -Original Message- > > From: Darren Hart [mailto:dvh...@infradead.org] > > Sent: Monday, July 9, 2018 4:07 PM > > To: Limonciello, Mario > > Cc: srinivas.pandruv...@linux.intel.com; andy.shevche...@gmail.com; > > alex.h...@canonical.com; a...@infradead.org; platform-driver- > > x...@vger.kernel.org; linux-kernel@vger.kernel.org; r...@rjwysocki.net > > Subject: Re: [PATCH v2] platform/x86: intel-hid: Add support for Device > > Specific > > Methods > > > > On Mon, Jul 09, 2018 at 04:38:16AM +, mario.limoncie...@dell.com wrote: > > > > -Original Message- > > > > From: Srinivas Pandruvada [mailto:srinivas.pandruv...@linux.intel.com] > > > > Sent: Saturday, July 7, 2018 8:44 AM > > > > To: Darren Hart; Andy Shevchenko > > > > Cc: Limonciello, Mario; Alex Hung; Andy Shevchenko; Platform Driver; > > > > Linux > > Kernel > > > > Mailing List; Rafael J. Wysocki > > > > Subject: Re: [PATCH v2] platform/x86: intel-hid: Add support for Device > > > > Specific > > > > Methods > > > > > > > > On Fri, 2018-07-06 at 16:59 -0700, Darren Hart wrote: > > > > > On Mon, Jul 02, 2018 at 05:06:14PM +0300, Andy Shevchenko wrote: > > > > > > On Mon, Jul 2, 2018 at 4:51 PM, > > > > > > wrote: > > ... > > > > Mario can add more. > > > > But I think Dell has released a BIOS fix, so that power button can > > > > still work using non _DSM way. So I think we can wait for normal > > > > release cycle. > > > > > > Yeah, I added some comments on a separate reply already indicating this. > > > > > > The affected system we know about has reverted the new interface. > > > > > > This all stems from an expectation that the _DSM has been there for "many" > > > releases on the Windows side. Long enough that even all the corporate > > downgrade > > > scenarios to older Windows versions and the drivers with them all worked > > properly. > > > > > > If this doesn't end up being a candidate for backporting to stable we'll > > > probably > > > end up asking our various OS partners to backport it as a SAUCE type > > > patch in > > distro > > > kernels to eventually be able to turn this back on. > > > > Thanks Mario. We'll skip the RC. We can discuss -stable as a separate > > issue. This patch fixes a demonstrable bug, the biggest obstacle here > > is the size. At 240 lines with context, it more than doubles the maximum > > patch size for stable. > > > > My recommendation is that we treat the stable backport of this as a > > special case, e.g. Andy and I don't tag it, but you or Srinivas propose > > it and specifically make the case for why an exception should be made to > > Greg. > > > > How self contained and isolated this change is will be an important part > > of the argument. On the one hand, it's all one file, on the other hand, > > this is a fairly generic driver in use in more and more platforms, with > > the potential for widespread impact if this introduces a new bug. > > > > In that case, how about we let it bake for a while and maybe by the time > 4.19 comes out is when we do the proposal of a stable backport? Works for me. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods
On Mon, Jul 09, 2018 at 09:08:56PM +, mario.limoncie...@dell.com wrote: > > -Original Message- > > From: Darren Hart [mailto:dvh...@infradead.org] > > Sent: Monday, July 9, 2018 4:07 PM > > To: Limonciello, Mario > > Cc: srinivas.pandruv...@linux.intel.com; andy.shevche...@gmail.com; > > alex.h...@canonical.com; a...@infradead.org; platform-driver- > > x...@vger.kernel.org; linux-kernel@vger.kernel.org; r...@rjwysocki.net > > Subject: Re: [PATCH v2] platform/x86: intel-hid: Add support for Device > > Specific > > Methods > > > > On Mon, Jul 09, 2018 at 04:38:16AM +, mario.limoncie...@dell.com wrote: > > > > -Original Message- > > > > From: Srinivas Pandruvada [mailto:srinivas.pandruv...@linux.intel.com] > > > > Sent: Saturday, July 7, 2018 8:44 AM > > > > To: Darren Hart; Andy Shevchenko > > > > Cc: Limonciello, Mario; Alex Hung; Andy Shevchenko; Platform Driver; > > > > Linux > > Kernel > > > > Mailing List; Rafael J. Wysocki > > > > Subject: Re: [PATCH v2] platform/x86: intel-hid: Add support for Device > > > > Specific > > > > Methods > > > > > > > > On Fri, 2018-07-06 at 16:59 -0700, Darren Hart wrote: > > > > > On Mon, Jul 02, 2018 at 05:06:14PM +0300, Andy Shevchenko wrote: > > > > > > On Mon, Jul 2, 2018 at 4:51 PM, > > > > > > wrote: > > ... > > > > Mario can add more. > > > > But I think Dell has released a BIOS fix, so that power button can > > > > still work using non _DSM way. So I think we can wait for normal > > > > release cycle. > > > > > > Yeah, I added some comments on a separate reply already indicating this. > > > > > > The affected system we know about has reverted the new interface. > > > > > > This all stems from an expectation that the _DSM has been there for "many" > > > releases on the Windows side. Long enough that even all the corporate > > downgrade > > > scenarios to older Windows versions and the drivers with them all worked > > properly. > > > > > > If this doesn't end up being a candidate for backporting to stable we'll > > > probably > > > end up asking our various OS partners to backport it as a SAUCE type > > > patch in > > distro > > > kernels to eventually be able to turn this back on. > > > > Thanks Mario. We'll skip the RC. We can discuss -stable as a separate > > issue. This patch fixes a demonstrable bug, the biggest obstacle here > > is the size. At 240 lines with context, it more than doubles the maximum > > patch size for stable. > > > > My recommendation is that we treat the stable backport of this as a > > special case, e.g. Andy and I don't tag it, but you or Srinivas propose > > it and specifically make the case for why an exception should be made to > > Greg. > > > > How self contained and isolated this change is will be an important part > > of the argument. On the one hand, it's all one file, on the other hand, > > this is a fairly generic driver in use in more and more platforms, with > > the potential for widespread impact if this introduces a new bug. > > > > In that case, how about we let it bake for a while and maybe by the time > 4.19 comes out is when we do the proposal of a stable backport? Works for me. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] dell-laptop: Fix backlight detection
On Sun, Jul 08, 2018 at 05:02:20PM +0200, Damien Thébault wrote:
> Fix return code check for "max brightness" ACPI call.
>
> The dell laptop ACPI video brightness control is not present on dell
> laptops anymore, but was present in older kernel versions.
>
> The code that checks the return value is incorrect since the SMM
> refactoring.
>
> The old code was:
> if (buffer->output[0] == 0)
>
> Which was changed to:
> ret = dell_send_request(...)
> if (ret)
>
> However, dell_send_request() will return 0 if buffer->output[0] == 0,
> so we must change the check to:
> if (ret == 0)
>
> This issue was found on a Dell M4800 laptop, and the fix tested on it
> as well.
>
> Fixes: 549b4930f057 ("dell-smbios: Introduce dispatcher for SMM calls")
> Signed-off-by: Damien Thébault
> Tested-by: Damien Thébault
Thanks for the analysis and fix Damien.
I was unable to apply the patch. Apparently due to your Content-Tranfer-Encoding
being set to "quoted-printable". This converts "=" characters to =3D.
Please work with your mail client and try sending the patch
to yourself and applying it. Once that works, please resend.
I suspect you may need to do one or both of the following:
- Disable preferences->compose->"wrap pasted text" (or whichever applies to how
you get the patch into the compose window)
- Set Preferences->Mail Handling->Sending->Transfer Encoding to 8bit to avoid
"quoted-printable".
Other claws users who send patches have Content-Transfer-Encoding set to 7bit
Thanks,
> ---
> drivers/platform/x86/dell-laptop.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/dell-laptop.c
> b/drivers/platform/x86/dell-laptop.c index f1fa8612db40..06978c14c83b
> 100644 --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -2185,7 +2185,7 @@ static int __init dell_init(void)
> dell_fill_request(, token->location, 0, 0, 0);
> ret = dell_send_request(,
> CLASS_TOKEN_READ,
> SELECT_TOKEN_AC);
> - if (ret)
> + if (ret == 0)
> max_intensity = buffer.output[3];
> }
>
> --
> 2.17.1
>
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] dell-laptop: Fix backlight detection
On Sun, Jul 08, 2018 at 05:02:20PM +0200, Damien Thébault wrote:
> Fix return code check for "max brightness" ACPI call.
>
> The dell laptop ACPI video brightness control is not present on dell
> laptops anymore, but was present in older kernel versions.
>
> The code that checks the return value is incorrect since the SMM
> refactoring.
>
> The old code was:
> if (buffer->output[0] == 0)
>
> Which was changed to:
> ret = dell_send_request(...)
> if (ret)
>
> However, dell_send_request() will return 0 if buffer->output[0] == 0,
> so we must change the check to:
> if (ret == 0)
>
> This issue was found on a Dell M4800 laptop, and the fix tested on it
> as well.
>
> Fixes: 549b4930f057 ("dell-smbios: Introduce dispatcher for SMM calls")
> Signed-off-by: Damien Thébault
> Tested-by: Damien Thébault
Thanks for the analysis and fix Damien.
I was unable to apply the patch. Apparently due to your Content-Tranfer-Encoding
being set to "quoted-printable". This converts "=" characters to =3D.
Please work with your mail client and try sending the patch
to yourself and applying it. Once that works, please resend.
I suspect you may need to do one or both of the following:
- Disable preferences->compose->"wrap pasted text" (or whichever applies to how
you get the patch into the compose window)
- Set Preferences->Mail Handling->Sending->Transfer Encoding to 8bit to avoid
"quoted-printable".
Other claws users who send patches have Content-Transfer-Encoding set to 7bit
Thanks,
> ---
> drivers/platform/x86/dell-laptop.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/dell-laptop.c
> b/drivers/platform/x86/dell-laptop.c index f1fa8612db40..06978c14c83b
> 100644 --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -2185,7 +2185,7 @@ static int __init dell_init(void)
> dell_fill_request(, token->location, 0, 0, 0);
> ret = dell_send_request(,
> CLASS_TOKEN_READ,
> SELECT_TOKEN_AC);
> - if (ret)
> + if (ret == 0)
> max_intensity = buffer.output[3];
> }
>
> --
> 2.17.1
>
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] platform/x86/toshiba_acpi.c: fix defined but not used build warnings
On Fri, Jul 06, 2018 at 08:53:09PM -0700, Randy Dunlap wrote: > From: Randy Dunlap > > Fix a build warning in toshiba_acpi.c when CONFIG_PROC_FS is not enabled > by marking the unused function as __maybe_unused. > > ../drivers/platform/x86/toshiba_acpi.c:1685:12: warning: 'version_proc_show' > defined but not used [-Wunused-function] > > Signed-off-by: Randy Dunlap > Cc: Azael Avalos > Cc: platform-driver-...@vger.kernel.org > Cc: Darren Hart > Cc: Andy Shevchenko Thanks Randy, queued. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] platform/x86/toshiba_acpi.c: fix defined but not used build warnings
On Fri, Jul 06, 2018 at 08:53:09PM -0700, Randy Dunlap wrote: > From: Randy Dunlap > > Fix a build warning in toshiba_acpi.c when CONFIG_PROC_FS is not enabled > by marking the unused function as __maybe_unused. > > ../drivers/platform/x86/toshiba_acpi.c:1685:12: warning: 'version_proc_show' > defined but not used [-Wunused-function] > > Signed-off-by: Randy Dunlap > Cc: Azael Avalos > Cc: platform-driver-...@vger.kernel.org > Cc: Darren Hart > Cc: Andy Shevchenko Thanks Randy, queued. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods
On Mon, Jul 09, 2018 at 04:38:16AM +, mario.limoncie...@dell.com wrote: > > -Original Message- > > From: Srinivas Pandruvada [mailto:srinivas.pandruv...@linux.intel.com] > > Sent: Saturday, July 7, 2018 8:44 AM > > To: Darren Hart; Andy Shevchenko > > Cc: Limonciello, Mario; Alex Hung; Andy Shevchenko; Platform Driver; Linux > > Kernel > > Mailing List; Rafael J. Wysocki > > Subject: Re: [PATCH v2] platform/x86: intel-hid: Add support for Device > > Specific > > Methods > > > > On Fri, 2018-07-06 at 16:59 -0700, Darren Hart wrote: > > > On Mon, Jul 02, 2018 at 05:06:14PM +0300, Andy Shevchenko wrote: > > > > On Mon, Jul 2, 2018 at 4:51 PM, > > > > wrote: ... > > Mario can add more. > > But I think Dell has released a BIOS fix, so that power button can > > still work using non _DSM way. So I think we can wait for normal > > release cycle. > > Yeah, I added some comments on a separate reply already indicating this. > > The affected system we know about has reverted the new interface. > > This all stems from an expectation that the _DSM has been there for "many" > releases on the Windows side. Long enough that even all the corporate > downgrade > scenarios to older Windows versions and the drivers with them all worked > properly. > > If this doesn't end up being a candidate for backporting to stable we'll > probably > end up asking our various OS partners to backport it as a SAUCE type patch in > distro > kernels to eventually be able to turn this back on. Thanks Mario. We'll skip the RC. We can discuss -stable as a separate issue. This patch fixes a demonstrable bug, the biggest obstacle here is the size. At 240 lines with context, it more than doubles the maximum patch size for stable. My recommendation is that we treat the stable backport of this as a special case, e.g. Andy and I don't tag it, but you or Srinivas propose it and specifically make the case for why an exception should be made to Greg. How self contained and isolated this change is will be an important part of the argument. On the one hand, it's all one file, on the other hand, this is a fairly generic driver in use in more and more platforms, with the potential for widespread impact if this introduces a new bug. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods
On Mon, Jul 09, 2018 at 04:38:16AM +, mario.limoncie...@dell.com wrote: > > -Original Message- > > From: Srinivas Pandruvada [mailto:srinivas.pandruv...@linux.intel.com] > > Sent: Saturday, July 7, 2018 8:44 AM > > To: Darren Hart; Andy Shevchenko > > Cc: Limonciello, Mario; Alex Hung; Andy Shevchenko; Platform Driver; Linux > > Kernel > > Mailing List; Rafael J. Wysocki > > Subject: Re: [PATCH v2] platform/x86: intel-hid: Add support for Device > > Specific > > Methods > > > > On Fri, 2018-07-06 at 16:59 -0700, Darren Hart wrote: > > > On Mon, Jul 02, 2018 at 05:06:14PM +0300, Andy Shevchenko wrote: > > > > On Mon, Jul 2, 2018 at 4:51 PM, > > > > wrote: ... > > Mario can add more. > > But I think Dell has released a BIOS fix, so that power button can > > still work using non _DSM way. So I think we can wait for normal > > release cycle. > > Yeah, I added some comments on a separate reply already indicating this. > > The affected system we know about has reverted the new interface. > > This all stems from an expectation that the _DSM has been there for "many" > releases on the Windows side. Long enough that even all the corporate > downgrade > scenarios to older Windows versions and the drivers with them all worked > properly. > > If this doesn't end up being a candidate for backporting to stable we'll > probably > end up asking our various OS partners to backport it as a SAUCE type patch in > distro > kernels to eventually be able to turn this back on. Thanks Mario. We'll skip the RC. We can discuss -stable as a separate issue. This patch fixes a demonstrable bug, the biggest obstacle here is the size. At 240 lines with context, it more than doubles the maximum patch size for stable. My recommendation is that we treat the stable backport of this as a special case, e.g. Andy and I don't tag it, but you or Srinivas propose it and specifically make the case for why an exception should be made to Greg. How self contained and isolated this change is will be an important part of the argument. On the one hand, it's all one file, on the other hand, this is a fairly generic driver in use in more and more platforms, with the potential for widespread impact if this introduces a new bug. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods
On Mon, Jul 02, 2018 at 05:06:14PM +0300, Andy Shevchenko wrote: > On Mon, Jul 2, 2018 at 4:51 PM, wrote: > > >> > So there are some customers who will have issue with power button > >> > without this patch, so it should be also marked for stable also. > >> > Also this may be a candidate for 4.18-rcX. > >> > > >> > >> I'm not sure Greg will take this selling point for rather big patch. > >> From changelog, honestly, I don't see any regression description, > >> looks like "it wasn't working before change anyway". > >> > > > > Just for adding some context. > > > > Some platforms have moved to different interface in ASL in FW upgrade > > due to deficiencies/bugs present with old interface. So yes it's platform > > FW > > change in behavior that leads to Linux kernel regression. Windows driver > > has supported > > both interfaces for a long time. Linux kernel however doesn't support this > > interface until now. > > > >> For now, I pushed this to my review and testing queue as is, thanks! > > > > If not stable I think it would at least be ideal to try to bring this to > > 4.18-rcX if possible for > > compatibility with more platforms that will come with this other interface > > instead. > > Citing Linus: > > --- 8< --- 8< --- > So please, people, the "fixes" during the rc series really should be > things that are _regressions_. If it used to work, and it no longer does, > then fixing that is a good and proper fix. Or if something oopses or has a > security implication, then the fix for that is a real fix. > > But if it's something that has never worked, even if it "fixes" some > behavior, then it's new development, and that should come in during the > merge window. Just because you think it's a "fix" doesn't mean that it > really is one, at least in the "during the rc series" sense. > --- 8< --- 8< --- > > So, if we can sell him that it used to work and firmware fix is a > Linux regression I'm fine. > > Darren, what do you think? So if I understand this correctly, we have this timeline. Linux v4.16 - Machine A works Linux v4.17 - Machine A works - Machine A updates firmware - Machine A stops working Linux v4.18 - Machine A still doesn't work So it is not a *Linux kernel* regression. The patch is too large for standard stable rules. It is a regression from any user's perspective - the machine worked, they followed good digital hygiene and updated their firmware, and now it doesn't. This user will now think twice before they update their BIOS again, since it may fundamentally change the platform, rather than committing to only fixing things below the OS Interface layer. :-( The risk of course is that this introduces new bugs - and as with anything that still uses _DSM (sigh, why?) that is quite possible due to the opaque interface. Very unfortunate to see _DSM ADDED to a previously _DSM free implementation. Linus is right, this is not a fix, this is feature development. I strongly advocate for vendors to have more control over their drivers, but this scenario really frustrates me. I don't think I can justify this to Linus as a fix. But before we just say "no" (because hey, I want these fixes available as early as possible too), let's ask Rafael if he has an opinion or if there is precedent for this in his experience with ACPI drivers in general: Rafael? Finally, we can also just ask Linus. The firmware update broke the power button, we can get it working again by supporting the new API with this patch... see what he says. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods
On Mon, Jul 02, 2018 at 05:06:14PM +0300, Andy Shevchenko wrote: > On Mon, Jul 2, 2018 at 4:51 PM, wrote: > > >> > So there are some customers who will have issue with power button > >> > without this patch, so it should be also marked for stable also. > >> > Also this may be a candidate for 4.18-rcX. > >> > > >> > >> I'm not sure Greg will take this selling point for rather big patch. > >> From changelog, honestly, I don't see any regression description, > >> looks like "it wasn't working before change anyway". > >> > > > > Just for adding some context. > > > > Some platforms have moved to different interface in ASL in FW upgrade > > due to deficiencies/bugs present with old interface. So yes it's platform > > FW > > change in behavior that leads to Linux kernel regression. Windows driver > > has supported > > both interfaces for a long time. Linux kernel however doesn't support this > > interface until now. > > > >> For now, I pushed this to my review and testing queue as is, thanks! > > > > If not stable I think it would at least be ideal to try to bring this to > > 4.18-rcX if possible for > > compatibility with more platforms that will come with this other interface > > instead. > > Citing Linus: > > --- 8< --- 8< --- > So please, people, the "fixes" during the rc series really should be > things that are _regressions_. If it used to work, and it no longer does, > then fixing that is a good and proper fix. Or if something oopses or has a > security implication, then the fix for that is a real fix. > > But if it's something that has never worked, even if it "fixes" some > behavior, then it's new development, and that should come in during the > merge window. Just because you think it's a "fix" doesn't mean that it > really is one, at least in the "during the rc series" sense. > --- 8< --- 8< --- > > So, if we can sell him that it used to work and firmware fix is a > Linux regression I'm fine. > > Darren, what do you think? So if I understand this correctly, we have this timeline. Linux v4.16 - Machine A works Linux v4.17 - Machine A works - Machine A updates firmware - Machine A stops working Linux v4.18 - Machine A still doesn't work So it is not a *Linux kernel* regression. The patch is too large for standard stable rules. It is a regression from any user's perspective - the machine worked, they followed good digital hygiene and updated their firmware, and now it doesn't. This user will now think twice before they update their BIOS again, since it may fundamentally change the platform, rather than committing to only fixing things below the OS Interface layer. :-( The risk of course is that this introduces new bugs - and as with anything that still uses _DSM (sigh, why?) that is quite possible due to the opaque interface. Very unfortunate to see _DSM ADDED to a previously _DSM free implementation. Linus is right, this is not a fix, this is feature development. I strongly advocate for vendors to have more control over their drivers, but this scenario really frustrates me. I don't think I can justify this to Linus as a fix. But before we just say "no" (because hey, I want these fixes available as early as possible too), let's ask Rafael if he has an opinion or if there is precedent for this in his experience with ACPI drivers in general: Rafael? Finally, we can also just ask Linus. The firmware update broke the power button, we can get it working again by supporting the new API with this patch... see what he says. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] platform/x86: sony-laptop: Mark expected switch fall-through
On Thu, Jul 05, 2018 at 03:47:03PM -0500, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
>
> Signed-off-by: Gustavo A. R. Silva
> ---
> drivers/platform/x86/sony-laptop.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/platform/x86/sony-laptop.c
> b/drivers/platform/x86/sony-laptop.c
> index b205b03..e614cb7 100644
> --- a/drivers/platform/x86/sony-laptop.c
> +++ b/drivers/platform/x86/sony-laptop.c
> @@ -4427,6 +4427,7 @@ sony_pic_read_possible_resource(struct acpi_resource
> *resource, void *context)
> default:
> dprintk("Resource %d isn't an IRQ nor an IO port\n",
> resource->type);
> + /* fall through */
Here too, I wonder if this is intentional. Either way, from what I can see, the
final line in the function:
return AE_CTRL_TERMINATE;
Is unreachable as there are no "break" statements in the switch, and the default
falls through to return AE_OK. Something doesn't seem right here.
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] platform/x86: sony-laptop: Mark expected switch fall-through
On Thu, Jul 05, 2018 at 03:47:03PM -0500, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
>
> Signed-off-by: Gustavo A. R. Silva
> ---
> drivers/platform/x86/sony-laptop.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/platform/x86/sony-laptop.c
> b/drivers/platform/x86/sony-laptop.c
> index b205b03..e614cb7 100644
> --- a/drivers/platform/x86/sony-laptop.c
> +++ b/drivers/platform/x86/sony-laptop.c
> @@ -4427,6 +4427,7 @@ sony_pic_read_possible_resource(struct acpi_resource
> *resource, void *context)
> default:
> dprintk("Resource %d isn't an IRQ nor an IO port\n",
> resource->type);
> + /* fall through */
Here too, I wonder if this is intentional. Either way, from what I can see, the
final line in the function:
return AE_CTRL_TERMINATE;
Is unreachable as there are no "break" statements in the switch, and the default
falls through to return AE_OK. Something doesn't seem right here.
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] platform/x86: acer-wmi: mark expected switch fall-throughs
On Thu, Jul 05, 2018 at 03:42:21PM -0500, Gustavo A. R. Silva wrote: > In preparation to enabling -Wimplicit-fallthrough, mark switch cases > where we are expecting to fall through. > > Warning level 2 was used: -Wimplicit-fallthrough=2 > > Signed-off-by: Gustavo A. R. Silva > --- > drivers/platform/x86/acer-wmi.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c > index 8952173..114b028 100644 > --- a/drivers/platform/x86/acer-wmi.c > +++ b/drivers/platform/x86/acer-wmi.c > @@ -1018,6 +1018,7 @@ static acpi_status WMID_get_u32(u32 *value, u32 cap) > *value = tmp & 0x1; > return 0; > } > + /* else: fall through */ > default: > return AE_ERROR; > } > @@ -1344,6 +1345,7 @@ static acpi_status get_u32(u32 *value, u32 cap) > status = AMW0_get_u32(value, cap); > break; > } > + /* else: fall through */ > case ACER_WMID: > status = WMID_get_u32(value, cap); > break; > @@ -1386,6 +1388,7 @@ static acpi_status set_u32(u32 value, u32 cap) > > return AMW0_set_u32(value, cap); > } > + /* else: fall through */ I suspect you are correct, bu these last two weren't obviously intentional to me. Has this seen any testing? -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] platform/x86: acer-wmi: mark expected switch fall-throughs
On Thu, Jul 05, 2018 at 03:42:21PM -0500, Gustavo A. R. Silva wrote: > In preparation to enabling -Wimplicit-fallthrough, mark switch cases > where we are expecting to fall through. > > Warning level 2 was used: -Wimplicit-fallthrough=2 > > Signed-off-by: Gustavo A. R. Silva > --- > drivers/platform/x86/acer-wmi.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c > index 8952173..114b028 100644 > --- a/drivers/platform/x86/acer-wmi.c > +++ b/drivers/platform/x86/acer-wmi.c > @@ -1018,6 +1018,7 @@ static acpi_status WMID_get_u32(u32 *value, u32 cap) > *value = tmp & 0x1; > return 0; > } > + /* else: fall through */ > default: > return AE_ERROR; > } > @@ -1344,6 +1345,7 @@ static acpi_status get_u32(u32 *value, u32 cap) > status = AMW0_get_u32(value, cap); > break; > } > + /* else: fall through */ > case ACER_WMID: > status = WMID_get_u32(value, cap); > break; > @@ -1386,6 +1388,7 @@ static acpi_status set_u32(u32 value, u32 cap) > > return AMW0_set_u32(value, cap); > } > + /* else: fall through */ I suspect you are correct, bu these last two weren't obviously intentional to me. Has this seen any testing? -- Darren Hart VMware Open Source Technology Center
Re: [PATCH 1/2] platform/x86: wmi: prefix sysfs files in /sys/bus/wmi with the ACPI device
On Sat, Dec 09, 2017 at 01:32:29PM -0800, Andy Lutomirski wrote: > On Fri, Dec 8, 2017 at 7:41 PM, wrote: > >>> On Dec 8, 2017, at 6:34 PM, Mario Limonciello > >>> wrote: > >>> > >>> It's possible for the same GUID to show up on as system twice. > >>> This means using solely the GUID for identify the file will not > >>> be sufficient. > >> > >>Isn't the file already in a per-bus directory? > > > > Yep, but the symlink created in /sys/bus/wmi/devices isn't. > > That's where the kernel complains about duplicate sysfs > > attributes. > > > > It's not exactly a pretty path I submitted, but it does avoid > > those collisions. > > > > Example (with this in place from /sys/bus/wmi/devices): > > lrwxrwxrwx 1 root root 0 Dec 8 21:39 > > PNP0C14:04-70FE8229-D03B-4214-A1C6-1F884B1A892A -> > > ../../../devices/platform/PNP0C14:04/wmi_bus/wmi_bus-PNP0C14:04/PNP0C14:04-70FE8229-D03B-4214-A1C6-1F884B1A892A > > Right, I saw that in the cover letter right after sending this. > > Greg, is there a cleaner way to deal with this? There are two > instances of the same bus type, each of which would like to have a > device called "70FE8229-D03B-4214-A1C6-1F884B1A892A". Can we somehow > rename the symlinks without renaming the device, or are we just > supposed to prefix the device name like Mario is doing here? > Mario, This one has been lingering at the bottom of my patch stack for a while now - sorry for not poking on this earlier. Is this still something you have a need for? If so, would you resend and add Greg to Cc - specifically directing Andy's question above to him? -- Darren Hart VMware Open Source Technology Center
Re: [PATCH 1/2] platform/x86: wmi: prefix sysfs files in /sys/bus/wmi with the ACPI device
On Sat, Dec 09, 2017 at 01:32:29PM -0800, Andy Lutomirski wrote: > On Fri, Dec 8, 2017 at 7:41 PM, wrote: > >>> On Dec 8, 2017, at 6:34 PM, Mario Limonciello > >>> wrote: > >>> > >>> It's possible for the same GUID to show up on as system twice. > >>> This means using solely the GUID for identify the file will not > >>> be sufficient. > >> > >>Isn't the file already in a per-bus directory? > > > > Yep, but the symlink created in /sys/bus/wmi/devices isn't. > > That's where the kernel complains about duplicate sysfs > > attributes. > > > > It's not exactly a pretty path I submitted, but it does avoid > > those collisions. > > > > Example (with this in place from /sys/bus/wmi/devices): > > lrwxrwxrwx 1 root root 0 Dec 8 21:39 > > PNP0C14:04-70FE8229-D03B-4214-A1C6-1F884B1A892A -> > > ../../../devices/platform/PNP0C14:04/wmi_bus/wmi_bus-PNP0C14:04/PNP0C14:04-70FE8229-D03B-4214-A1C6-1F884B1A892A > > Right, I saw that in the cover letter right after sending this. > > Greg, is there a cleaner way to deal with this? There are two > instances of the same bus type, each of which would like to have a > device called "70FE8229-D03B-4214-A1C6-1F884B1A892A". Can we somehow > rename the symlinks without renaming the device, or are we just > supposed to prefix the device name like Mario is doing here? > Mario, This one has been lingering at the bottom of my patch stack for a while now - sorry for not poking on this earlier. Is this still something you have a need for? If so, would you resend and add Greg to Cc - specifically directing Andy's question above to him? -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] platform/x86: dell-smbios: make a function and a pointer static
On Sat, Jun 23, 2018 at 08:24:26AM +0200, Julia Lawall wrote:
>
>
> On Fri, 22 Jun 2018, Darren Hart wrote:
>
> > On Thu, Jun 21, 2018 at 07:15:24PM +0100, Colin King wrote:
> > > From: Colin Ian King
> > >
> > > The function dell_smbios_smm_call and pointer platform_device are
> > > local to the source and do not need to be in global scope, so make
> > > them static.
> > >
> > > Cleans up sparse warnings:
> > > warning: symbol 'platform_device' was not declared. Should it be static?
> > > warning: symbol 'dell_smbios_smm_call' was not declared. Should it be
> > > static?
> > >
> > > Signed-off-by: Colin Ian King
> > > ---
> > > drivers/platform/x86/dell-smbios-smm.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/dell-smbios-smm.c
> > > b/drivers/platform/x86/dell-smbios-smm.c
> > > index e9e9da556318..97a90bebc360 100644
> > > --- a/drivers/platform/x86/dell-smbios-smm.c
> > > +++ b/drivers/platform/x86/dell-smbios-smm.c
> > > @@ -24,7 +24,7 @@
> > > static int da_command_address;
> > > static int da_command_code;
> > > static struct calling_interface_buffer *buffer;
> > > -struct platform_device *platform_device;
> > > +static struct platform_device *platform_device;
> > > static DEFINE_MUTEX(smm_mutex);
> > >
> > > static const struct dmi_system_id dell_device_table[] __initconst = {
> > > @@ -82,7 +82,7 @@ static void find_cmd_address(const struct dmi_header
> > > *dm, void *dummy)
> > > }
> > > }
> > >
> > > -int dell_smbios_smm_call(struct calling_interface_buffer *input)
> > > +static int dell_smbios_smm_call(struct calling_interface_buffer *input)
> >
> > Hrm. So these are passed by pointer to dell_smbios_register_device(), which
> > is in
> > turn called by dell_smbios_call() from dell-smbios-base.c.
> >
> > So while it is valid to make these static, since we're not referencing the
> > symbol, but the pointer value instead - I do worry about the "static"
> > suggesting
> > to someone reading the code that this data is not used outside of this file,
> > when it is.
>
> Static protects the name. The name in this case is very generic.
Indeed "platform_device" is even more generic than I'd like for a static ;-)
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] platform/x86: dell-smbios: make a function and a pointer static
On Sat, Jun 23, 2018 at 08:24:26AM +0200, Julia Lawall wrote:
>
>
> On Fri, 22 Jun 2018, Darren Hart wrote:
>
> > On Thu, Jun 21, 2018 at 07:15:24PM +0100, Colin King wrote:
> > > From: Colin Ian King
> > >
> > > The function dell_smbios_smm_call and pointer platform_device are
> > > local to the source and do not need to be in global scope, so make
> > > them static.
> > >
> > > Cleans up sparse warnings:
> > > warning: symbol 'platform_device' was not declared. Should it be static?
> > > warning: symbol 'dell_smbios_smm_call' was not declared. Should it be
> > > static?
> > >
> > > Signed-off-by: Colin Ian King
> > > ---
> > > drivers/platform/x86/dell-smbios-smm.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/dell-smbios-smm.c
> > > b/drivers/platform/x86/dell-smbios-smm.c
> > > index e9e9da556318..97a90bebc360 100644
> > > --- a/drivers/platform/x86/dell-smbios-smm.c
> > > +++ b/drivers/platform/x86/dell-smbios-smm.c
> > > @@ -24,7 +24,7 @@
> > > static int da_command_address;
> > > static int da_command_code;
> > > static struct calling_interface_buffer *buffer;
> > > -struct platform_device *platform_device;
> > > +static struct platform_device *platform_device;
> > > static DEFINE_MUTEX(smm_mutex);
> > >
> > > static const struct dmi_system_id dell_device_table[] __initconst = {
> > > @@ -82,7 +82,7 @@ static void find_cmd_address(const struct dmi_header
> > > *dm, void *dummy)
> > > }
> > > }
> > >
> > > -int dell_smbios_smm_call(struct calling_interface_buffer *input)
> > > +static int dell_smbios_smm_call(struct calling_interface_buffer *input)
> >
> > Hrm. So these are passed by pointer to dell_smbios_register_device(), which
> > is in
> > turn called by dell_smbios_call() from dell-smbios-base.c.
> >
> > So while it is valid to make these static, since we're not referencing the
> > symbol, but the pointer value instead - I do worry about the "static"
> > suggesting
> > to someone reading the code that this data is not used outside of this file,
> > when it is.
>
> Static protects the name. The name in this case is very generic.
Indeed "platform_device" is even more generic than I'd like for a static ;-)
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] platform/x86: dell-smbios: make a function and a pointer static
On Tue, Jun 26, 2018 at 10:32:17PM +0300, Andy Shevchenko wrote: > On Sat, Jun 23, 2018 at 3:22 AM, Darren Hart wrote: > > On Thu, Jun 21, 2018 at 07:15:24PM +0100, Colin King wrote: > >> From: Colin Ian King > >> > >> The function dell_smbios_smm_call and pointer platform_device are > >> local to the source and do not need to be in global scope, so make > >> them static. > >> > >> Cleans up sparse warnings: > >> warning: symbol 'platform_device' was not declared. Should it be static? > >> warning: symbol 'dell_smbios_smm_call' was not declared. Should it be > >> static? > > > >> -int dell_smbios_smm_call(struct calling_interface_buffer *input) > >> +static int dell_smbios_smm_call(struct calling_interface_buffer *input) > > > > Hrm. So these are passed by pointer to dell_smbios_register_device(), which > > is in > > turn called by dell_smbios_call() from dell-smbios-base.c. > > > > So while it is valid to make these static, since we're not referencing the > > symbol, but the pointer value instead - I do worry about the "static" > > suggesting > > to someone reading the code that this data is not used outside of this file, > > when it is. > > > > I'm not finding a position on this in coding-style. > > > > Andy, do you care to weigh in on this? > > We are using static keyword by almost all callback defined functions, > so, for my point of view it's pretty much okay. OK, just wanted to double check. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] platform/x86: dell-smbios: make a function and a pointer static
On Tue, Jun 26, 2018 at 10:32:17PM +0300, Andy Shevchenko wrote: > On Sat, Jun 23, 2018 at 3:22 AM, Darren Hart wrote: > > On Thu, Jun 21, 2018 at 07:15:24PM +0100, Colin King wrote: > >> From: Colin Ian King > >> > >> The function dell_smbios_smm_call and pointer platform_device are > >> local to the source and do not need to be in global scope, so make > >> them static. > >> > >> Cleans up sparse warnings: > >> warning: symbol 'platform_device' was not declared. Should it be static? > >> warning: symbol 'dell_smbios_smm_call' was not declared. Should it be > >> static? > > > >> -int dell_smbios_smm_call(struct calling_interface_buffer *input) > >> +static int dell_smbios_smm_call(struct calling_interface_buffer *input) > > > > Hrm. So these are passed by pointer to dell_smbios_register_device(), which > > is in > > turn called by dell_smbios_call() from dell-smbios-base.c. > > > > So while it is valid to make these static, since we're not referencing the > > symbol, but the pointer value instead - I do worry about the "static" > > suggesting > > to someone reading the code that this data is not used outside of this file, > > when it is. > > > > I'm not finding a position on this in coding-style. > > > > Andy, do you care to weigh in on this? > > We are using static keyword by almost all callback defined functions, > so, for my point of view it's pretty much okay. OK, just wanted to double check. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] platform/x86: dell-smbios: make a function and a pointer static
On Thu, Jun 21, 2018 at 07:15:24PM +0100, Colin King wrote:
> From: Colin Ian King
>
> The function dell_smbios_smm_call and pointer platform_device are
> local to the source and do not need to be in global scope, so make
> them static.
>
> Cleans up sparse warnings:
> warning: symbol 'platform_device' was not declared. Should it be static?
> warning: symbol 'dell_smbios_smm_call' was not declared. Should it be
> static?
>
> Signed-off-by: Colin Ian King
> ---
> drivers/platform/x86/dell-smbios-smm.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/dell-smbios-smm.c
> b/drivers/platform/x86/dell-smbios-smm.c
> index e9e9da556318..97a90bebc360 100644
> --- a/drivers/platform/x86/dell-smbios-smm.c
> +++ b/drivers/platform/x86/dell-smbios-smm.c
> @@ -24,7 +24,7 @@
> static int da_command_address;
> static int da_command_code;
> static struct calling_interface_buffer *buffer;
> -struct platform_device *platform_device;
> +static struct platform_device *platform_device;
> static DEFINE_MUTEX(smm_mutex);
>
> static const struct dmi_system_id dell_device_table[] __initconst = {
> @@ -82,7 +82,7 @@ static void find_cmd_address(const struct dmi_header *dm,
> void *dummy)
> }
> }
>
> -int dell_smbios_smm_call(struct calling_interface_buffer *input)
> +static int dell_smbios_smm_call(struct calling_interface_buffer *input)
Hrm. So these are passed by pointer to dell_smbios_register_device(), which is
in
turn called by dell_smbios_call() from dell-smbios-base.c.
So while it is valid to make these static, since we're not referencing the
symbol, but the pointer value instead - I do worry about the "static" suggesting
to someone reading the code that this data is not used outside of this file,
when it is.
I'm not finding a position on this in coding-style.
Andy, do you care to weigh in on this?
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] platform/x86: dell-smbios: make a function and a pointer static
On Thu, Jun 21, 2018 at 07:15:24PM +0100, Colin King wrote:
> From: Colin Ian King
>
> The function dell_smbios_smm_call and pointer platform_device are
> local to the source and do not need to be in global scope, so make
> them static.
>
> Cleans up sparse warnings:
> warning: symbol 'platform_device' was not declared. Should it be static?
> warning: symbol 'dell_smbios_smm_call' was not declared. Should it be
> static?
>
> Signed-off-by: Colin Ian King
> ---
> drivers/platform/x86/dell-smbios-smm.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/dell-smbios-smm.c
> b/drivers/platform/x86/dell-smbios-smm.c
> index e9e9da556318..97a90bebc360 100644
> --- a/drivers/platform/x86/dell-smbios-smm.c
> +++ b/drivers/platform/x86/dell-smbios-smm.c
> @@ -24,7 +24,7 @@
> static int da_command_address;
> static int da_command_code;
> static struct calling_interface_buffer *buffer;
> -struct platform_device *platform_device;
> +static struct platform_device *platform_device;
> static DEFINE_MUTEX(smm_mutex);
>
> static const struct dmi_system_id dell_device_table[] __initconst = {
> @@ -82,7 +82,7 @@ static void find_cmd_address(const struct dmi_header *dm,
> void *dummy)
> }
> }
>
> -int dell_smbios_smm_call(struct calling_interface_buffer *input)
> +static int dell_smbios_smm_call(struct calling_interface_buffer *input)
Hrm. So these are passed by pointer to dell_smbios_register_device(), which is
in
turn called by dell_smbios_call() from dell-smbios-base.c.
So while it is valid to make these static, since we're not referencing the
symbol, but the pointer value instead - I do worry about the "static" suggesting
to someone reading the code that this data is not used outside of this file,
when it is.
I'm not finding a position on this in coding-style.
Andy, do you care to weigh in on this?
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] platform/x86: ideapad-laptop: Apply no_hw_rfkill to Y20-15IKBM, too
On Fri, Jun 22, 2018 at 10:59:17AM +0200, Takashi Iwai wrote:
> The commit 5d9f40b56630 ("platform/x86: ideapad-laptop: Add
> Y520-15IKBN to no_hw_rfkill") added the entry for Y20-15IKBN, and it
> turned out that another variant, Y20-15IKBM, also requires the
> no_hw_rfkill.
>
> Trim the last letter from the string so that it matches to both
> Y20-15IKBN and Y20-15IKBM models.
>
> Bugzilla: https://bugzilla.opensuse.org/show_bug.cgi?id=1098626
> Cc:
> Signed-off-by: Takashi Iwai
Thanks Takashi, queued.
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] platform/x86: ideapad-laptop: Apply no_hw_rfkill to Y20-15IKBM, too
On Fri, Jun 22, 2018 at 10:59:17AM +0200, Takashi Iwai wrote:
> The commit 5d9f40b56630 ("platform/x86: ideapad-laptop: Add
> Y520-15IKBN to no_hw_rfkill") added the entry for Y20-15IKBN, and it
> turned out that another variant, Y20-15IKBM, also requires the
> no_hw_rfkill.
>
> Trim the last letter from the string so that it matches to both
> Y20-15IKBN and Y20-15IKBM models.
>
> Bugzilla: https://bugzilla.opensuse.org/show_bug.cgi?id=1098626
> Cc:
> Signed-off-by: Takashi Iwai
Thanks Takashi, queued.
--
Darren Hart
VMware Open Source Technology Center
Re: [PATCH] platform/x86: dell-smbios-base: Support systems without tokens
On Fri, Jun 15, 2018 at 12:12:51PM -0500, Mario Limonciello wrote: > Some Dell servers can use dell-smbios but they don't support the > token interface. Make it optional. > > Signed-off-by: Mario Limonciello Thanks Mario, queued. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] platform/x86: dell-smbios-base: Support systems without tokens
On Fri, Jun 15, 2018 at 12:12:51PM -0500, Mario Limonciello wrote: > Some Dell servers can use dell-smbios but they don't support the > token interface. Make it optional. > > Signed-off-by: Mario Limonciello Thanks Mario, queued. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] platform/x86: wmi: Do not mix pages and kmalloc
On Thu, Jun 21, 2018 at 01:24:34AM +, mario.limoncie...@dell.com wrote: > > -Original Message- > > From: Darren Hart [mailto:dvh...@infradead.org] > > Sent: Wednesday, June 20, 2018 7:17 PM > > To: Kees Cook > > Cc: LKML; Andy Shevchenko; Platform Driver; Mihai Donțu; Limonciello, Mario > > Subject: Re: [PATCH] platform/x86: wmi: Do not mix pages and kmalloc > > > > On Wed, Jun 20, 2018 at 04:43:14PM -0700, Kees Cook wrote: > > > On Wed, Jun 20, 2018 at 4:37 PM, Darren Hart wrote: > > > > On Wed, Jun 20, 2018 at 02:31:41PM -0700, Kees Cook wrote: > > > >> The probe handler_data was being allocated with __get_free_pages() > > > >> for no reason I could find. The error path was using kfree(). Since > > > > > > > > v4 of Mario's series used kmalloc: > > > > https://patchwork.kernel.org/patch/9985827/ > > > > > > > > This was changed in v10 to use __get_free_pages: > > > > https://patchwork.kernel.org/patch/10018023/ > > > > > > > > But... I'm not finding the discussion that led to this change Mario, > > > > do you recall? Something about contiguous memory? We had a similar > > > > discussion on an earlier series: > > > > > > > > https://patchwork.kernel.org/patch/9975277/ > > > > > > FWIW, kmalloc gets you contiguous memory... > > > > Yeah, I'm not finding a valid reason to use __get_free_pages over kmalloc in > > this case. I'll give Mario a chance to respond in case I'm just missing > > something, but otherwise will plan to apply this patch. > > I think it was for contiguous memory, so if kmalloc is giving that I agree > no need to keep __get_free_pages instead. > > Acked-by: Mario Limonciello Confirmed, kmalloc in physically contiguous. Queued up, and tagged for stable. Thanks everyone. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] platform/x86: wmi: Do not mix pages and kmalloc
On Thu, Jun 21, 2018 at 01:24:34AM +, mario.limoncie...@dell.com wrote: > > -Original Message- > > From: Darren Hart [mailto:dvh...@infradead.org] > > Sent: Wednesday, June 20, 2018 7:17 PM > > To: Kees Cook > > Cc: LKML; Andy Shevchenko; Platform Driver; Mihai Donțu; Limonciello, Mario > > Subject: Re: [PATCH] platform/x86: wmi: Do not mix pages and kmalloc > > > > On Wed, Jun 20, 2018 at 04:43:14PM -0700, Kees Cook wrote: > > > On Wed, Jun 20, 2018 at 4:37 PM, Darren Hart wrote: > > > > On Wed, Jun 20, 2018 at 02:31:41PM -0700, Kees Cook wrote: > > > >> The probe handler_data was being allocated with __get_free_pages() > > > >> for no reason I could find. The error path was using kfree(). Since > > > > > > > > v4 of Mario's series used kmalloc: > > > > https://patchwork.kernel.org/patch/9985827/ > > > > > > > > This was changed in v10 to use __get_free_pages: > > > > https://patchwork.kernel.org/patch/10018023/ > > > > > > > > But... I'm not finding the discussion that led to this change Mario, > > > > do you recall? Something about contiguous memory? We had a similar > > > > discussion on an earlier series: > > > > > > > > https://patchwork.kernel.org/patch/9975277/ > > > > > > FWIW, kmalloc gets you contiguous memory... > > > > Yeah, I'm not finding a valid reason to use __get_free_pages over kmalloc in > > this case. I'll give Mario a chance to respond in case I'm just missing > > something, but otherwise will plan to apply this patch. > > I think it was for contiguous memory, so if kmalloc is giving that I agree > no need to keep __get_free_pages instead. > > Acked-by: Mario Limonciello Confirmed, kmalloc in physically contiguous. Queued up, and tagged for stable. Thanks everyone. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH] platform/x86: wmi: Do not mix pages and kmalloc
On Wed, Jun 20, 2018 at 04:43:14PM -0700, Kees Cook wrote: > On Wed, Jun 20, 2018 at 4:37 PM, Darren Hart wrote: > > On Wed, Jun 20, 2018 at 02:31:41PM -0700, Kees Cook wrote: > >> The probe handler_data was being allocated with __get_free_pages() > >> for no reason I could find. The error path was using kfree(). Since > > > > v4 of Mario's series used kmalloc: > > https://patchwork.kernel.org/patch/9985827/ > > > > This was changed in v10 to use __get_free_pages: > > https://patchwork.kernel.org/patch/10018023/ > > > > But... I'm not finding the discussion that led to this change Mario, > > do you recall? Something about contiguous memory? We had a similar > > discussion on an earlier series: > > > > https://patchwork.kernel.org/patch/9975277/ > > FWIW, kmalloc gets you contiguous memory... Yeah, I'm not finding a valid reason to use __get_free_pages over kmalloc in this case. I'll give Mario a chance to respond in case I'm just missing something, but otherwise will plan to apply this patch. > > But if the reason is found and needs to stay, the probe error path's > kfree() needs to be fixed, and __GFP_COMP needs to be added to the > free page flags. Got it, thanks Kees. -- Darren Hart VMware Open Source Technology Center
