linux-kernel - Re: [PATCH v9 11/14] platform/x86: Split the alienware-wmi driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c314c485-7a6f-b10a-2d80-45a8c5fb331e@linux.intel.com>
Date: Mon, 10 Feb 2025 13:53:05 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Kurt Borja <kuurtb@...il.com>
cc: platform-driver-x86@...r.kernel.org, Armin Wolf <W_Armin@....de>, 
    Mario Limonciello <mario.limonciello@....com>, 
    Hans de Goede <hdegoede@...hat.com>, Dell.Client.Kernel@...l.com, 
    LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v9 11/14] platform/x86: Split the alienware-wmi driver

On Fri, 7 Feb 2025, Kurt Borja wrote:

> On Fri Feb 7, 2025 at 10:05 AM -05, Ilpo Järvinen wrote:
> > On Fri, 7 Feb 2025, Kurt Borja wrote:
> >
> >> Split alienware-wmi WMI drivers into different files. This is done
> >> seamlessly by copying and pasting, however some blocks are reordered.
> >> 
> >> Reviewed-by: Armin Wolf <W_Armin@....de>
> >> Reviewed-by: Mario Limonciello <mario.limonciello@....com>
> >> Signed-off-by: Kurt Borja <kuurtb@...il.com>
> >
> > Hi,
> >
> > Can you please check there's no error in driver_data assignments as the 
> > numbers in removed & added lines do not match:
> 
> Hi Ilpo,
> 
> There was indeed a wrong assignment to Alienware m16 r1 AMD, I'm not
> really sure how it happened but it's fixed now!
> 
> I'll send a v10. I apologize for the noise.
> 
> >
> > $ git diff-tree -p 73224c076cf2fa2968d61584c62937f6180c8e71 | grep driver_data | rev | sort | rev | uniq -c
> 
> Thanks for this amazing trick btw.

Apparently the rev trick wouldn't have been even necessary in this case 
but writing those things are a second nature to me. When reviewing 
especially move changes, one has to have a bag full of tricks. :-)

It is one of the reasons why I prefer to have move patches do as little 
extra work as possible because I can use pipelines to verify the pre and 
post content is identical.

I usually starting by diffing - and + lines in the diff which is how I 
came across this one too. In the best case there are no code line changes 
at all but all changes are in the boilerplate, it gives very high 
confidence on the move being done correctly. When a rebase conflicts, 
everyone (me included), might introduce unintended changes to move-only 
patches so I tend to check even my own move patches in similar fashion to 
avoid making stupid mistakes.

Even the diff of diffs wouldn't get me to 100%, it find that <5% that 
differs so I can focus on whether it is sound.

> 
> ~ Kurt
> 
> >       1 +               awcc = id->driver_data;
> >       1 -               awcc = id->driver_data;
> >       4 +               .driver_data = &generic_quirks,
> >       5 -               .driver_data = &generic_quirks,
> >       7 +               .driver_data = &g_series_quirks,
> >       6 -               .driver_data = &g_series_quirks,
> >
> > (That commit id is from my staging tree, not available to you but it's 
> > this patch.)
> 

-- 
 i.