Re: [GIT PULL] platform-drivers-x86 for 4.15-1
On Mon, Nov 20, 2017 at 09:48:58PM -1000, Linus Torvalds wrote: > On Mon, Nov 20, 2017 at 9:06 AM, Darren Hart wrote: > > > > Back in the 4.2 timeframe, platform-drivers-x86-v4.2-2 specifically, I > > started adding my pull request commentary to the tag directly and the > > pull requests themselves tended to have little or no information beyond > > that. > > Right - that's fine. I don't actually care whether the information is > in the signed tag, or in the emailed pull request, or in both. I'll > take it either way. > > There are valid reasons to put it in the signed tag - that way you > write it as you do the tag, and then "git pull-request" is pretty much > entirely just automation. > > But some people prefer to do the tag as just a marker (so the tag > message may be basically worthless), and then write the explanation > later in the email as they send it off. And that's fine too. > > And yet other people do both - write some summary in the tag, but hen > write more about it in the emailed message. And I'll take that too, no > problem. > > And in all three cases I'll edit things for grammar and whitespace > (indentation) etc, and may remove commentary that may be interesting > for me doing the merge, but isn't relevant in the history once the > merge is done. > > > I didn't see a clear division between what should go in the pull > > request email body and what should be in the tag, this kept all the > > information about the pull request together in the tag. > > There really is no division at all. One common _pattern_ is to have > the email message contain more of a freeform message, while the tag > contains more of a "summary list", but that's just a pattern that some > people tend to use, and it's not in any way universal or required. > > > Andy's pull request follows this pattern, with the text of the tag > > opening with the summary and context relevant to the pull request before > > the munged shortlog: > > Yes, and that separation is fine. > > But I do want both parts to make sense. If the munged shortlog is pure > automation, why send it to me at all? Or if you do send it to me, say > that it's just filler for _you_, not for me. > > But it looks like useful information, but without human editing, it's not. > > It's basically the difference between "data" and "information". I want > information, and if it's pure data that I could get from "git > shortlog" that I should just ignore, then tell me to ignore it. Thanks Linus, this is helpful. Andy and I have updated our tooling to reflect the above, and we will modify our human-information-add as follows: The pull-request body will include merge-specific information and instructions which are unrelated to the content. e.g. previously merged fixes, pre-merged immutable branches, recommended merge conflict resolution. The tag message will include a human written highlights and summary, followed by a git shortlog grouped by driver with a prefix indicating it is automatically generated (although we will still verify the script grouped things properly). This just offers a quick glance at what changed by driver. I will also sometimes group fixes from static analysis that would otherwise be rather noisy under a common group, e.g. sparse fixes: - Updated several drivers to use const strings -- Darren Hart VMware Open Source Technology Center
Re: [GIT PULL] platform-drivers-x86 for 4.15-1
On Mon, Nov 20, 2017 at 9:06 AM, Darren Hart wrote: > > Back in the 4.2 timeframe, platform-drivers-x86-v4.2-2 specifically, I > started adding my pull request commentary to the tag directly and the > pull requests themselves tended to have little or no information beyond > that. Right - that's fine. I don't actually care whether the information is in the signed tag, or in the emailed pull request, or in both. I'll take it either way. There are valid reasons to put it in the signed tag - that way you write it as you do the tag, and then "git pull-request" is pretty much entirely just automation. But some people prefer to do the tag as just a marker (so the tag message may be basically worthless), and then write the explanation later in the email as they send it off. And that's fine too. And yet other people do both - write some summary in the tag, but hen write more about it in the emailed message. And I'll take that too, no problem. And in all three cases I'll edit things for grammar and whitespace (indentation) etc, and may remove commentary that may be interesting for me doing the merge, but isn't relevant in the history once the merge is done. > I didn't see a clear division between what should go in the pull > request email body and what should be in the tag, this kept all the > information about the pull request together in the tag. There really is no division at all. One common _pattern_ is to have the email message contain more of a freeform message, while the tag contains more of a "summary list", but that's just a pattern that some people tend to use, and it's not in any way universal or required. > Andy's pull request follows this pattern, with the text of the tag > opening with the summary and context relevant to the pull request before > the munged shortlog: Yes, and that separation is fine. But I do want both parts to make sense. If the munged shortlog is pure automation, why send it to me at all? Or if you do send it to me, say that it's just filler for _you_, not for me. But it looks like useful information, but without human editing, it's not. It's basically the difference between "data" and "information". I want information, and if it's pure data that I could get from "git shortlog" that I should just ignore, then tell me to ignore it. Linus
Re: [GIT PULL] platform-drivers-x86 for 4.15-1
On Sat, Nov 18, 2017 at 10:37:55AM -0800, Linus Torvalds wrote: > On Sat, Nov 18, 2017 at 10:09 AM, Andy Shevchenko > wrote: > > > > Here is the collected material against Platform Drivers x86 subsystem. > > It's rather bit busy cycle for PDx86, mostly due to Dell SMBIOS driver > > activity. > > So I note that you seem to use the same summary script that Darren used. > > Guys, the "merge summary" should be human-readable, not just a > slightly munged version of "git shortlog" that has been sorted by > first word. > > Feel free to use that munged version as a base-line, but don't just > send me automated merge messages. > > The whole point of a summary is to summarize. And merge messages > should be legible, not "this is all the details that you could have > just used 'git shortlog' to get". Hi Linus, Back in the 4.2 timeframe, platform-drivers-x86-v4.2-2 specifically, I started adding my pull request commentary to the tag directly and the pull requests themselves tended to have little or no information beyond that. I didn't see a clear division between what should go in the pull request email body and what should be in the tag, this kept all the information about the pull request together in the tag. Andy's pull request follows this pattern, with the text of the tag opening with the summary and context relevant to the pull request before the munged shortlog: --- 8< --- For this cycle we have quite an update for the Dell SMBIOS driver including WMI work to provide an interface for SMBIOS tokens via sysfs and WMI support for 2017+ Dell laptop models. SMM dispatcher code is split into a separate driver followed by a new WMI dispatcher. The latter provides a character device interface to user space. The pull request contains a merge of immutable branch from Wolfram Sang in order to apply a dependent fix to the Intel CherryTrail Battery Management driver. Other Intel drivers got a lot of cleanups. The Turbo Boost Max 3.0 support is added for Intel Skylake. Peaq WMI hotkeys driver gets its own maintainer and white list of supported models. Silead DMI is expanded to support few additional platforms. Tablet mode via GMMS ACPI method is added to support some ThinkPad tablets. Two commits appear here which were previously merged during the v4.14-rcX cycle: - d7ca5ebf2493 platform/x86: intel_pmc_ipc: Use devm_* calls in driver probe function - e3075fd6f80c platform/x86: intel_pmc_ipc: Use spin_lock to protect GCR updates --- 8< --- Do you not want to see this in the tag and prefer we move it to the GIT PULL email body? If so, what do you want to see in the tag message? Or, is the above not what you're looking for at all? And if not, what are you looking for instead? > > When you can't even be bothered to fix up obvious issues like > > (a) "yeah, I just sorted things alphabetically without any human > interaction": > > intel-wmi-thunderbolt: > - Silence error cases > > MAINTAINERS: > - Add entry for the PEAQ WMI hotkeys driver > > mlx-platform: > - make a couple of structures static > > (b) "yeah, my summary script splits the overview at a colon, so when > there is no colon it doesn't work": > > Add driver to force WMI Thunderbolt controller power status: > - Add driver to force WMI Thunderbolt controller power status > > it's good that you have a fairly unified commit message model and can > do these summaries, but it's bad when you then don't do much of any > _human_ summary at all or even check the result for sanity. Agreed. Andy, we need to read through everything we put into that pull request and adjust to make sense. Pull Requests are not automated, the scripts are there to help us avoid human error, and be as consistent as possible. -- Darren Hart VMware Open Source Technology Center
Re: [GIT PULL] platform-drivers-x86 for 4.15-1
On Sat, Nov 18, 2017 at 12:09:10PM -0800, Linus Torvalds wrote: > On Sat, Nov 18, 2017 at 10:37 AM, Linus Torvalds > wrote: > > > > So I note that you seem to use the same summary script that Darren used. > > .. oh, and I note a *much* worse issue. > > You add new drivers and then default them to "on". > > THAT IS COMPLETELY UNACCEPTABLE. > > I don't know why I have to say this every single merge window, but > let's do it one more time: > > As a developer, you think _your_ driver or feature is the most > important thing ever, and you have the hardware. > > AND ALMOST NOBODY ELSE CARES. > > Read it and weep. Unless your hardware is completely ubiquitous, it > damn well should not default to being defaulted everybody elses > config. Understood and agreed, this is especially true for our subsystem which is full of platform specific drivers. > > In particular, people who do "make oldconfig" clearly had a > configuration _without_ your hardware and were happy with it, and want > to keep it working. That's what "oldconfig" means. > > You don't say "hey, let's enable this piece of hardware that you don't > have anyway, just to waste your time and disk and memory". > > So the things that merit "default y/m" are > > (a) you added a Kconfig option for something that used to always be > built. Then it merits that "default y" exactly because "make > oldconfig" should just work. > > (b) corollary of the above: if you add a new gatekeeping Kconfig > option that hides/shows other Kconfig options (but doesn't generate > any code of its own), it should be enabled by default, simply so that > by default people will see those other options. > > (c) your driver itself defaults to off, but you then have sub-driver > options for behavior or similar, where you can give sane defaults for > people who _do_ have your hardware, and want the driver for it, and > within those constraints the extended option makes sense > > (d) your piece of hardware or infrastructure really is something that > everybody expects. If you have CONFIG_NET or CONFIG_BLOCK, you get to > enable it by default. > > But something like CONFIG_DELL_SMBIOS sure as hell does not merit > being default on. Not even if you have enabled WMI. > > EVERY SINGLE "default" line that got added by this branch was wrong. > > Stop doing this. It's a serious violation of peoples expectations. > When I do "make oldconfig", I don't want some new random hardware > support. The above looks like good Documentation/ material. A quick scan doesn't find it, but I'll look more closely and prepare a patch adding it if I don't find it. I'll have a sidebar with Andy and we'll review, set expectations, update tooling as necessary, and resubmit. Given the above Kconfig default, we'll prepare a patch on top of the existing HEAD to default to No, and create a new tag. Appreciate the detail above, we'll make sure it doesn't get lost. -- Darren Hart VMware Open Source Technology Center
Re: [GIT PULL] platform-drivers-x86 for 4.15-1
On Sat, Nov 18, 2017 at 10:37 AM, Linus Torvalds wrote: > > So I note that you seem to use the same summary script that Darren used. .. oh, and I note a *much* worse issue. You add new drivers and then default them to "on". THAT IS COMPLETELY UNACCEPTABLE. I don't know why I have to say this every single merge window, but let's do it one more time: As a developer, you think _your_ driver or feature is the most important thing ever, and you have the hardware. AND ALMOST NOBODY ELSE CARES. Read it and weep. Unless your hardware is completely ubiquitous, it damn well should not default to being defaulted everybody elses config. In particular, people who do "make oldconfig" clearly had a configuration _without_ your hardware and were happy with it, and want to keep it working. That's what "oldconfig" means. You don't say "hey, let's enable this piece of hardware that you don't have anyway, just to waste your time and disk and memory". So the things that merit "default y/m" are (a) you added a Kconfig option for something that used to always be built. Then it merits that "default y" exactly because "make oldconfig" should just work. (b) corollary of the above: if you add a new gatekeeping Kconfig option that hides/shows other Kconfig options (but doesn't generate any code of its own), it should be enabled by default, simply so that by default people will see those other options. (c) your driver itself defaults to off, but you then have sub-driver options for behavior or similar, where you can give sane defaults for people who _do_ have your hardware, and want the driver for it, and within those constraints the extended option makes sense (d) your piece of hardware or infrastructure really is something that everybody expects. If you have CONFIG_NET or CONFIG_BLOCK, you get to enable it by default. But something like CONFIG_DELL_SMBIOS sure as hell does not merit being default on. Not even if you have enabled WMI. EVERY SINGLE "default" line that got added by this branch was wrong. Stop doing this. It's a serious violation of peoples expectations. When I do "make oldconfig", I don't want some new random hardware support. Linus
Re: [GIT PULL] platform-drivers-x86 for 4.15-1
On Sat, Nov 18, 2017 at 10:09 AM, Andy Shevchenko wrote: > > Here is the collected material against Platform Drivers x86 subsystem. > It's rather bit busy cycle for PDx86, mostly due to Dell SMBIOS driver > activity. So I note that you seem to use the same summary script that Darren used. Guys, the "merge summary" should be human-readable, not just a slightly munged version of "git shortlog" that has been sorted by first word. Feel free to use that munged version as a base-line, but don't just send me automated merge messages. The whole point of a summary is to summarize. And merge messages should be legible, not "this is all the details that you could have just used 'git shortlog' to get". When you can't even be bothered to fix up obvious issues like (a) "yeah, I just sorted things alphabetically without any human interaction": intel-wmi-thunderbolt: - Silence error cases MAINTAINERS: - Add entry for the PEAQ WMI hotkeys driver mlx-platform: - make a couple of structures static (b) "yeah, my summary script splits the overview at a colon, so when there is no colon it doesn't work": Add driver to force WMI Thunderbolt controller power status: - Add driver to force WMI Thunderbolt controller power status it's good that you have a fairly unified commit message model and can do these summaries, but it's bad when you then don't do much of any _human_ summary at all or even check the result for sanity. Linus
[GIT PULL] platform-drivers-x86 for 4.15-1
Hi Linus, Here is the collected material against Platform Drivers x86 subsystem. It's rather bit busy cycle for PDx86, mostly due to Dell SMBIOS driver activity. During merge it will get a conflict CONFLICT (content): Merge conflict in Documentation/admin-guide/thunderbolt.rst that is pretty straight forward to solve, i.e. we need all parts of documentation be in, where "Networking over Thunderbolt cable" is followed by "Forcing power" chapter. Thanks, With Best Regards, Andy Shevchenko The following changes since commit 0224d45c9d46401b6d7018a96cfe049c5da7d91c: i2c-cht-wc: Add device-properties for fusb302 integration (2017-10-27 15:51:51 +0200) are available in the Git repository at: git://git.infradead.org/linux-platform-drivers-x86.git tags/platform-drivers-x86-v4.15-1 for you to fetch changes up to aaa40965d2342137d756121993c395e2a7463a8d: platform/x86: silead_dmi: Add silead, home-button property to some tablets (2017-11-18 19:28:58 +0200) platform-drivers-x86 for v4.15-1 For this cycle we have quite an update for the Dell SMBIOS driver including WMI work to provide an interface for SMBIOS tokens via sysfs and WMI support for 2017+ Dell laptop models. SMM dispatcher code is split into a separate driver followed by a new WMI dispatcher. The latter provides a character device interface to user space. The pull request contains a merge of immutable branch from Wolfram Sang in order to apply a dependent fix to the Intel CherryTrail Battery Management driver. Other Intel drivers got a lot of cleanups. The Turbo Boost Max 3.0 support is added for Intel Skylake. Peaq WMI hotkeys driver gets its own maintainer and white list of supported models. Silead DMI is expanded to support few additional platforms. Tablet mode via GMMS ACPI method is added to support some ThinkPad tablets. Two commits appear here which were previously merged during the v4.14-rcX cycle: - d7ca5ebf2493 platform/x86: intel_pmc_ipc: Use devm_* calls in driver probe function - e3075fd6f80c platform/x86: intel_pmc_ipc: Use spin_lock to protect GCR updates Add driver to force WMI Thunderbolt controller power status: - Add driver to force WMI Thunderbolt controller power status asus-wmi: - Add lightbar led support dell-laptop: - Allocate buffer before rfkill use dell-smbios: - fix string overflow - Add filtering support - Introduce dispatcher for SMM calls - Add a sysfs interface for SMBIOS tokens - only run if proper oem string is detected - Prefix class/select with cmd_ - Add pr_fmt definition to driver dell-smbios-smm: - test for WSMT dell-smbios-wmi: - release mutex lock on WMI call failure - introduce userspace interface - Add new WMI dispatcher driver dell-smo8800: - remove redundant assignments to byte_data dell-wmi: - don't check length returned - clean up wmi descriptor check - increase severity of some failures - Do not match on descriptor GUID modalias - Label driver as handling notifications dell-*wmi*: - Relay failed initial probe to dependent drivers dell-wmi-descriptor: - check if memory was allocated - split WMI descriptor into it's own driver fujitsu-laptop: - Fix radio LED detection - Don't oops when FUJ02E3 is not presnt hp_accel: - Add quirk for HP ProBook 440 G4 hp-wmi: - Fix tablet mode detection for convertibles ideapad-laptop: - Add Lenovo Yoga 920-13IKB to no_hw_rfkill dmi list intel_cht_int33fe: - Update fusb302 type string, add properties - make a couple of local functions static - Work around BIOS bug on some devices intel-hid: - Power button suspend on Dell Latitude 7275 intel_ips: - Convert timers to use timer_setup() - Remove FSF address from GPL notice - Remove unneeded fields and label - Keep pointer to struct device - Use PCI_VDEVICE() macro - Switch to new PCI IRQ allocation API - Simplify error handling via devres API intel_pmc_ipc: - Revert Use MFD framework to create dependent devices - Use MFD framework to create dependent devices - Use spin_lock to protect GCR updates - Use devm_* calls in driver probe function intel_punit_ipc: - Fix resource ioremap warning intel_telemetry: - Remove useless default in Kconfig - Add needed inclusion - cleanup redundant headers - Fix typos - Fix load failure info intel_telemetry_debugfs: - Use standard ARRAY_SIZE() macro intel_turbo_max_3: - Add Skylake platform intel-wmi-thunderbolt: - Silence error cases MAINTAINERS: - Add entry for the PEAQ WMI hotkeys driver mlx-platform: - make a couple of structures static peaq_wmi: - Fix missing terminating entry for peaq_dmi_table peaq-wmi: - Remove unnecessary checks from peaq_wmi_exit - Add DMI check before binding to the WMI interface - Revert Blacklist Lenovo ideapad 700-15ISK - Blacklist Lenovo ideapad 700-15ISK silead_dmi: - Add silead, home-button property to some tablets - Add entry
